From 45da2790ae861a475d94f0b0ee819e3d643bd804 Mon Sep 17 00:00:00 2001 From: Caio Augusto Date: Wed, 30 Oct 2024 23:34:56 -0300 Subject: [PATCH 1/4] fix: replace "approvals_before_merge" with "approvals_left" when checking if pull is mergeable --- server/events/vcs/gitlab_client.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index fffe8c63e9..7c6c10f437 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -291,7 +291,7 @@ func (g *GitlabClient) PullIsApproved(logger logging.SimpleLogging, repo models. // PullIsMergeable returns true if the merge request can be merged. // In GitLab, there isn't a single field that tells us if the pull request is -// mergeable so for now we check the merge_status and approvals_before_merge +// mergeable so for now we check the merge_status and ApprovalsLeft // fields. // In order to check if the repo required these, we'd need to make another API // call to get the repo settings. @@ -360,6 +360,11 @@ func (g *GitlabClient) PullIsMergeable(logger logging.SimpleLogging, repo models logger.Debug("Merge status: '%s'", mr.MergeStatus) //nolint:staticcheck // Need to reference deprecated field for backwards compatibility } + approval, _, err := g.Client.MergeRequestApprovals.GetConfiguration(repo.FullName, pull.Num) + if err != nil { + return false, err + } + if ((supportsDetailedMergeStatus && (mr.DetailedMergeStatus == "mergeable" || mr.DetailedMergeStatus == "ci_still_running" || @@ -367,7 +372,7 @@ func (g *GitlabClient) PullIsMergeable(logger logging.SimpleLogging, repo models mr.DetailedMergeStatus == "need_rebase")) || (!supportsDetailedMergeStatus && mr.MergeStatus == "can_be_merged")) && //nolint:staticcheck // Need to reference deprecated field for backwards compatibility - mr.ApprovalsBeforeMerge <= 0 && + approval.ApprovalsLeft <= 0 && mr.BlockingDiscussionsResolved && !mr.WorkInProgress && (allowSkippedPipeline || !isPipelineSkipped) { From 747490ce2bc47223f3277293af2cbbf821ca5454 Mon Sep 17 00:00:00 2001 From: Caio Augusto Date: Wed, 30 Oct 2024 23:37:09 -0300 Subject: [PATCH 2/4] fix: typo --- server/events/vcs/gitlab_client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 7c6c10f437..b182ef35bf 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -291,7 +291,7 @@ func (g *GitlabClient) PullIsApproved(logger logging.SimpleLogging, repo models. // PullIsMergeable returns true if the merge request can be merged. // In GitLab, there isn't a single field that tells us if the pull request is -// mergeable so for now we check the merge_status and ApprovalsLeft +// mergeable so for now we check the merge_status and approvals_left // fields. // In order to check if the repo required these, we'd need to make another API // call to get the repo settings. @@ -372,7 +372,7 @@ func (g *GitlabClient) PullIsMergeable(logger logging.SimpleLogging, repo models mr.DetailedMergeStatus == "need_rebase")) || (!supportsDetailedMergeStatus && mr.MergeStatus == "can_be_merged")) && //nolint:staticcheck // Need to reference deprecated field for backwards compatibility - approval.ApprovalsLeft <= 0 && + approval.ApprovalsLeft == 0 && mr.BlockingDiscussionsResolved && !mr.WorkInProgress && (allowSkippedPipeline || !isPipelineSkipped) { From 59e921a10146e5ef85b5830e471ca25ab783f0ee Mon Sep 17 00:00:00 2001 From: Caio Augusto Date: Thu, 31 Oct 2024 00:15:16 -0300 Subject: [PATCH 3/4] chore: log gitlab client request response --- server/events/vcs/gitlab_client.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index b182ef35bf..123a5c6758 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -360,7 +360,10 @@ func (g *GitlabClient) PullIsMergeable(logger logging.SimpleLogging, repo models logger.Debug("Merge status: '%s'", mr.MergeStatus) //nolint:staticcheck // Need to reference deprecated field for backwards compatibility } - approval, _, err := g.Client.MergeRequestApprovals.GetConfiguration(repo.FullName, pull.Num) + approval, resp, err := g.Client.MergeRequestApprovals.GetConfiguration(repo.FullName, pull.Num) + if resp != nil { + logger.Debug("GET /projects/%d/merge_requests/%d/approvals returned: %d", mr.ProjectID, mr.IID, resp.StatusCode) + } if err != nil { return false, err } From f8e774891761bca996a747a039cc1566cf1ed61b Mon Sep 17 00:00:00 2001 From: Caio Augusto Date: Fri, 6 Dec 2024 21:43:14 -0300 Subject: [PATCH 4/4] fix(tests): fix broken tests for PullIsMergeable function --- server/events/vcs/gitlab_client_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index 8aee1e865a..10604d3ca9 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -816,6 +816,10 @@ func TestGitlabClient_PullIsMergeable(t *testing.T) { case fmt.Sprintf("/api/v4/projects/runatlantis%%2Fatlantis/merge_requests/%v", defaultMr): w.WriteHeader(http.StatusOK) w.Write(pipelineSuccess) // nolint: errcheck + case fmt.Sprintf("/api/v4/projects/runatlantis%%2Fatlantis/merge_requests/%v/approvals", c.mrID): + w.WriteHeader(http.StatusOK) + response := fmt.Sprintf(`{"id":%v,"name":"%s","rule_type":"regular","report_type":null,"eligible_approvers":[{"id":5,"name":"John Doe","username":"jdoe","state":"active","avatar_url":"https://www.gravatar.com/avatar/0?s=80&d=identicon","web_url":"http://localhost/jdoe"},{"id":50,"name":"Group Member 1","username":"group_member_1","state":"active","avatar_url":"https://www.gravatar.com/avatar/0?s=80&d=identicon","web_url":"http://localhost/group_member_1"}],"approvals_required":0,"users":[{"id":5,"name":"John Doe","username":"jdoe","state":"active","avatar_url":"https://www.gravatar.com/avatar/0?s=80&d=identicon","web_url":"http://localhost/jdoe"}],"contains_hidden_groups":false}`, c.mrID, c.statusName) + w.Write([]byte(response)) // nolint: errcheck case fmt.Sprintf("/api/v4/projects/runatlantis%%2Fatlantis/merge_requests/%v", noHeadPipelineMR): w.WriteHeader(http.StatusOK) w.Write(headPipelineNotAvailable) // nolint: errcheck