From 7650ac88ab66ed8a1d90b59c12345d4484db8de6 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Fri, 6 Oct 2023 23:41:10 -0400 Subject: [PATCH 1/2] feat: Clarify approved vs mergeable --- runatlantis.io/docs/command-requirements.md | 14 +-- server/events/command_requirement_handler.go | 6 +- .../command_requirement_handler_test.go | 6 +- server/events/project_command_runner_test.go | 4 +- server/events/vcs/gitlab_client.go | 15 +-- server/events/vcs/gitlab_client_test.go | 91 +++++++++++++++++++ .../gitlab-approved-merge-request.json | 37 ++++++++ .../gitlab-unapproved-merge-request.json | 41 +++++++++ 8 files changed, 190 insertions(+), 24 deletions(-) create mode 100644 server/events/vcs/testdata/gitlab-approved-merge-request.json create mode 100644 server/events/vcs/testdata/gitlab-unapproved-merge-request.json diff --git a/runatlantis.io/docs/command-requirements.md b/runatlantis.io/docs/command-requirements.md index e3aea4ea21..d48c6fe89e 100644 --- a/runatlantis.io/docs/command-requirements.md +++ b/runatlantis.io/docs/command-requirements.md @@ -43,16 +43,10 @@ The `approved` requirement by: ``` #### Meaning -Each VCS provider has different rules around who can approve: -* **GitHub** – **Any user with read permissions** to the repo can approve a pull request -* **GitLab** – The user who can approve can be set in the [repo settings](https://docs.gitlab.com/ee/user/project/merge_requests/approvals/) -* **Bitbucket Cloud (bitbucket.org)** – A user can approve their own pull request but - Atlantis does not count that as an approval and requires an approval from at least one user that - is not the author of the pull request -* **Azure DevOps** – **All builtin groups include the "Contribute to pull requests"** permission and can approve a pull request +Each VCS provider has different rules around what constitutes an "Approved" request. However, Atlantis `approved` is implemented in the same way for each VCS: at least one user other than the author who has the ability in the VCS to approve the request has done so. If you're using a VCS that has more complicated rules for determining approval, you can assert they are satisfied with the [mergeable](#mergeable) requirement. :::tip Tip -To require **certain people** to approve the pull request, look at the +To require **certain people** or a **certain number of people** to approve the pull request, look at the [mergeable](#mergeable) requirement. ::: @@ -87,7 +81,7 @@ Set the `mergeable` requirement by: ``` #### Meaning -Each VCS provider has a different concept of "mergeability": +Each VCS provider has a different concept of "mergeability". For some this includes satisfying "approval requirements", which differs from the more narrow [approved](#approved) requirement above. ::: warning Some VCS providers have a feature for branch protection to control "mergeability". To use it, @@ -119,7 +113,7 @@ If you set `atlantis/apply` to the mergeable requirement, use the `--gh-allow-me ::: #### GitLab -For GitLab, a merge request will be merged if there are no conflicts, no unresolved discussions if it is a project requirement and if all necessary approvers have approved the pull request. +For GitLab, a merge request will be merged if there are no conflicts, no unresolved discussions if it is a project requirement and if all necessary approvers have approved the pull request. The number or list of users that are required for approval can be set in the [repo settings](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html). For pipelines, if the project requires that pipelines must succeed, all builds except the apply command status will be checked. diff --git a/server/events/command_requirement_handler.go b/server/events/command_requirement_handler.go index 5c7b1c1d54..6ae156c6eb 100644 --- a/server/events/command_requirement_handler.go +++ b/server/events/command_requirement_handler.go @@ -26,7 +26,7 @@ func (a *DefaultCommandRequirementHandler) ValidatePlanProject(repoDir string, c switch req { case raw.ApprovedRequirement: if !ctx.PullReqStatus.ApprovalStatus.IsApproved { - return "Pull request must be approved according to the project's approval rules before running plan.", nil + return "Pull request must be approved by at least one person other than the author before running plan.", nil } case raw.MergeableRequirement: if !ctx.PullReqStatus.Mergeable { @@ -47,7 +47,7 @@ func (a *DefaultCommandRequirementHandler) ValidateApplyProject(repoDir string, switch req { case raw.ApprovedRequirement: if !ctx.PullReqStatus.ApprovalStatus.IsApproved { - return "Pull request must be approved according to the project's approval rules before running apply.", nil + return "Pull request must be approved by at least one person other than the author before running apply.", nil } // this should come before mergeability check since mergeability is a superset of this check. case valid.PoliciesPassedCommandReq: @@ -88,7 +88,7 @@ func (a *DefaultCommandRequirementHandler) ValidateImportProject(repoDir string, switch req { case raw.ApprovedRequirement: if !ctx.PullReqStatus.ApprovalStatus.IsApproved { - return "Pull request must be approved according to the project's approval rules before running import.", nil + return "Pull request must be approved by at least one person other than the author before running import.", nil } case raw.MergeableRequirement: if !ctx.PullReqStatus.Mergeable { diff --git a/server/events/command_requirement_handler_test.go b/server/events/command_requirement_handler_test.go index 1c737f05aa..2a29a7fa00 100644 --- a/server/events/command_requirement_handler_test.go +++ b/server/events/command_requirement_handler_test.go @@ -58,7 +58,7 @@ func TestAggregateApplyRequirements_ValidatePlanProject(t *testing.T) { ApprovalStatus: models.ApprovalStatus{IsApproved: false}, }, }, - wantFailure: "Pull request must be approved according to the project's approval rules before running plan.", + wantFailure: "Pull request must be approved by at least one person other than the author before running plan.", wantErr: assert.NoError, }, { @@ -142,7 +142,7 @@ func TestAggregateApplyRequirements_ValidateApplyProject(t *testing.T) { ApprovalStatus: models.ApprovalStatus{IsApproved: false}, }, }, - wantFailure: "Pull request must be approved according to the project's approval rules before running apply.", + wantFailure: "Pull request must be approved by at least one person other than the author before running apply.", wantErr: assert.NoError, }, { @@ -375,7 +375,7 @@ func TestAggregateApplyRequirements_ValidateImportProject(t *testing.T) { ApprovalStatus: models.ApprovalStatus{IsApproved: false}, }, }, - wantFailure: "Pull request must be approved according to the project's approval rules before running import.", + wantFailure: "Pull request must be approved by at least one person other than the author before running import.", wantErr: assert.NoError, }, { diff --git a/server/events/project_command_runner_test.go b/server/events/project_command_runner_test.go index 786b67088e..d82a912cd9 100644 --- a/server/events/project_command_runner_test.go +++ b/server/events/project_command_runner_test.go @@ -272,7 +272,7 @@ func TestDefaultProjectCommandRunner_ApplyNotApproved(t *testing.T) { When(mockWorkingDir.GetWorkingDir(ctx.BaseRepo, ctx.Pull, ctx.Workspace)).ThenReturn(tmp, nil) res := runner.Apply(ctx) - Equals(t, "Pull request must be approved according to the project's approval rules before running apply.", res.Failure) + Equals(t, "Pull request must be approved by at least one person other than the author before running apply.", res.Failure) } // Test that if mergeable is required and the PR isn't mergeable we give an error. @@ -676,7 +676,7 @@ func TestDefaultProjectCommandRunner_Import(t *testing.T) { IsApproved: false, }, }, - expFailure: "Pull request must be approved according to the project's approval rules before running import.", + expFailure: "Pull request must be approved by at least one person other than the author before running import.", }, } diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index d9e2b4d33c..95781002a1 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -270,7 +270,7 @@ func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, co return nil } -// PullIsApproved returns true if the merge request was approved. +// PullIsApproved returns true if the merge request was approved by someone other than the author func (g *GitlabClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) { approvals, resp, err := g.Client.MergeRequests.GetMergeRequestApprovals(repo.FullName, pull.Num) if resp != nil { @@ -279,12 +279,15 @@ func (g *GitlabClient) PullIsApproved(repo models.Repo, pull models.PullRequest) if err != nil { return approvalStatus, err } - if approvals.ApprovalsLeft > 0 { - return approvalStatus, nil + for _, approver := range approvals.ApprovedBy { + if approver.User.Username != pull.Author { + return models.ApprovalStatus{ + IsApproved: true, + ApprovedBy: approver.User.Username, + }, nil + } } - return models.ApprovalStatus{ - IsApproved: true, - }, nil + return approvalStatus, nil } // PullIsMergeable returns true if the merge request can be merged. diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index 99cf6b426e..5364b1962a 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -340,6 +340,97 @@ func TestGitlabClient_UpdateStatus(t *testing.T) { } } +func TestGitlabClient_PullIsApproved(t *testing.T) { + gitlabClientUnderTest = true + + unapprovedMergeRequest, err := os.ReadFile("testdata/gitlab-unapproved-merge-request.json") + Ok(t, err) + + approvedMergeRequest, err := os.ReadFile("testdata/gitlab-approved-merge-request.json") + Ok(t, err) + + unapprovedMR := 1 + approvedMR := 2 + cases := []struct { + name string + mrID int + author string + expState models.ApprovalStatus + }{ + { + "Unapproved", + unapprovedMR, + "lmassa", + models.ApprovalStatus{}, + }, + { + "Approved by author", + approvedMR, + "lmassa", + models.ApprovalStatus{}, + }, + { + "Approved by someone other than author", + approvedMR, + "someone-else", + models.ApprovalStatus{ + IsApproved: true, + ApprovedBy: "lmassa", + }, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + testServer := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/v4/": + // Rate limiter requests. + w.WriteHeader(http.StatusOK) + case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/1/approvals": + w.WriteHeader(http.StatusOK) + w.Write(unapprovedMergeRequest) // nolint: errcheck + case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/2/approvals": + w.WriteHeader(http.StatusOK) + w.Write(approvedMergeRequest) // nolint: errcheck + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + } + })) + + internalClient, err := gitlab.NewClient("token", gitlab.WithBaseURL(testServer.URL)) + Ok(t, err) + client := &GitlabClient{ + Client: internalClient, + Version: nil, + logger: logging.NewNoopLogger(t), + } + + repo := models.Repo{ + FullName: "runatlantis/atlantis", + Owner: "runatlantis", + Name: "atlantis", + VCSHost: models.VCSHost{ + Type: models.Gitlab, + Hostname: "gitlab.com", + }, + } + + approved, err := client.PullIsApproved(repo, models.PullRequest{ + Num: c.mrID, + BaseRepo: repo, + Author: c.author, + HeadCommit: "67cb91d3f6198189f433c045154a885784ba6977", + }) + + Ok(t, err) + Equals(t, c.expState, approved) + }) + } + +} + func TestGitlabClient_PullIsMergeable(t *testing.T) { gitlabClientUnderTest = true gitlabVersionOver15_6 := "15.8.3-ee" diff --git a/server/events/vcs/testdata/gitlab-approved-merge-request.json b/server/events/vcs/testdata/gitlab-approved-merge-request.json new file mode 100644 index 0000000000..68bcd3e542 --- /dev/null +++ b/server/events/vcs/testdata/gitlab-approved-merge-request.json @@ -0,0 +1,37 @@ +{ + "id": 545644, + "iid": 2, + "project_id": 17829, + "title": "More test", + "description": "", + "state": "opened", + "created_at": "2023-10-07T04:02:15.394Z", + "updated_at": "2023-10-07T04:44:20.860Z", + "merge_status": "can_be_merged", + "approved": true, + "approvals_required": 1, + "approvals_left": 0, + "require_password_to_approve": false, + "approved_by": [ + { + "user": { + "id": 81, + "username": "lmassa", + "name": "Luke Massa", + "state": "active", + "avatar_url": "https://secure.gravatar.com/avatar/0e6dd44205919a3dab626e54672a4141?s=80&d=identicon", + "web_url": "https://gitlab.dev.tripadvisor.com/lmassa" + } + } + ], + "suggested_approvers": [], + "approvers": [], + "approver_groups": [], + "user_has_approved": true, + "user_can_approve": false, + "approval_rules_left": [], + "has_approval_rules": true, + "merge_request_approvers_available": true, + "multiple_approval_rules_available": true, + "invalid_approvers_rules": [] +} diff --git a/server/events/vcs/testdata/gitlab-unapproved-merge-request.json b/server/events/vcs/testdata/gitlab-unapproved-merge-request.json new file mode 100644 index 0000000000..a5970ef0cb --- /dev/null +++ b/server/events/vcs/testdata/gitlab-unapproved-merge-request.json @@ -0,0 +1,41 @@ +{ + "id": 542185, + "iid": 1, + "project_id": 17829, + "title": "Update main.tf", + "description": "", + "state": "closed", + "created_at": "2023-09-27T02:50:38.784Z", + "updated_at": "2023-09-27T02:56:16.039Z", + "merge_status": "can_be_merged", + "approved": false, + "approvals_required": 1, + "approvals_left": 1, + "require_password_to_approve": false, + "approved_by": [], + "suggested_approvers": [ + { + "id": 81, + "username": "lmassa", + "name": "Luke Massa", + "state": "active", + "avatar_url": "https://secure.gravatar.com/avatar/0e6dd44205919a3dab626e54672a4141?s=80&d=identicon", + "web_url": "https://gitlab.dev.tripadvisor.com/lmassa" + } + ], + "approvers": [], + "approver_groups": [], + "user_has_approved": false, + "user_can_approve": true, + "approval_rules_left": [ + { + "id": 2204, + "name": "lmassa", + "rule_type": "regular" + } + ], + "has_approval_rules": true, + "merge_request_approvers_available": true, + "multiple_approval_rules_available": true, + "invalid_approvers_rules": [] +} From f6bb1b20a7e229e810cf572748c9ea460929b1f3 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Wed, 22 May 2024 20:08:25 -0400 Subject: [PATCH 2/2] Fix tests --- server/events/vcs/gitlab_client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index f3a84082aa..9eb9c69088 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -347,6 +347,7 @@ func TestGitlabClient_UpdateStatus(t *testing.T) { func TestGitlabClient_PullIsApproved(t *testing.T) { gitlabClientUnderTest = true + logger := logging.NewNoopLogger(t) unapprovedMergeRequest, err := os.ReadFile("testdata/gitlab-unapproved-merge-request.json") Ok(t, err) @@ -409,7 +410,6 @@ func TestGitlabClient_PullIsApproved(t *testing.T) { client := &GitlabClient{ Client: internalClient, Version: nil, - logger: logging.NewNoopLogger(t), } repo := models.Repo{ @@ -422,7 +422,7 @@ func TestGitlabClient_PullIsApproved(t *testing.T) { }, } - approved, err := client.PullIsApproved(repo, models.PullRequest{ + approved, err := client.PullIsApproved(logger, repo, models.PullRequest{ Num: c.mrID, BaseRepo: repo, Author: c.author,