feat: Clarify approved vs mergeable for gitlab

runatlantis.io/docs/command-requirements.md

@@ -53,18 +53,11 @@ The `approved` requirement by:
     ```
 
 #### Meaning
+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.
 
-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
 
 :::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.
 :::
 
@@ -106,8 +99,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,
@@ -141,8 +133,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.
 

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 {

server/events/command_requirement_handler_test.go

@@ -59,7 +59,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,
 		},
 		{
@@ -143,7 +143,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,
 		},
 		{
@@ -376,7 +376,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,
 		},
 		{

server/events/project_command_runner_test.go

@@ -260,7 +260,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.
@@ -674,7 +674,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.",
 		},
 	}
 

server/events/vcs/gitlab_client.go

@@ -271,7 +271,7 @@ func (g *GitlabClient) HidePrevCommandComments(logger logging.SimpleLogging, rep
 	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(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) {
 	logger.Debug("Checking if GitLab merge request %d is approved", pull.Num)
 	approvals, resp, err := g.Client.MergeRequests.GetMergeRequestApprovals(repo.FullName, pull.Num)
@@ -281,12 +281,15 @@ func (g *GitlabClient) PullIsApproved(logger logging.SimpleLogging, repo models.
 	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.

server/events/vcs/gitlab_client_test.go

@@ -345,6 +345,95 @@ 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)
+
+	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,
+			}
+
+			repo := models.Repo{
+				FullName: "runatlantis/atlantis",
+				Owner:    "runatlantis",
+				Name:     "atlantis",
+				VCSHost: models.VCSHost{
+					Type:     models.Gitlab,
+					Hostname: "gitlab.com",
+				},
+			}
+
+			approved, err := client.PullIsApproved(logger, repo, models.PullRequest{
+				Num:        c.mrID,
+				BaseRepo:   repo,
+				Author:     c.author,
+				HeadCommit: "67cb91d3f6198189f433c045154a885784ba6977",
+			})
+
+			Ok(t, err)
+			Equals(t, c.expState, approved)
+		})
+	}
+}
 func TestGitlabClient_UpdateStatusRetryable(t *testing.T) {
 	logger := logging.NewNoopLogger(t)
 	pipelineSuccess, err := os.ReadFile("testdata/gitlab-pipeline-success.json")

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": []
+}

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": []
+}