From df0d58d29b3fb5b1b19bf7ae115166a5b079b358 Mon Sep 17 00:00:00 2001 From: "Michel Z. Santello" Date: Thu, 16 Jun 2022 13:26:20 -0300 Subject: [PATCH] fix: vcs-status-name hardcoded in PullIsMergeable function (#2312) * feat: add userConfig.VCSStatusName to applyCommandRunner context * fix: use vcsstatusname from context * chore: normalize tests * chore: update interface mock --- .../events/events_controller_e2e_test.go | 3 ++- server/events/apply_command_runner.go | 5 ++++- server/events/command_runner_test.go | 4 +++- server/events/vcs/azuredevops_client.go | 2 +- server/events/vcs/azuredevops_client_test.go | 2 +- server/events/vcs/bitbucketcloud/client.go | 2 +- server/events/vcs/bitbucketcloud/client_test.go | 2 +- server/events/vcs/bitbucketserver/client.go | 2 +- server/events/vcs/client.go | 2 +- server/events/vcs/github_client.go | 6 +++--- server/events/vcs/github_client_test.go | 7 ++++--- server/events/vcs/gitlab_client.go | 6 +++--- server/events/vcs/gitlab_client_test.go | 13 +++++++------ server/events/vcs/instrumented_client.go | 4 ++-- server/events/vcs/mocks/mock_client.go | 2 +- server/events/vcs/not_configured_vcs_client.go | 2 +- server/events/vcs/proxy.go | 4 ++-- server/events/vcs/pull_status_fetcher.go | 6 +++--- server/server.go | 1 + 19 files changed, 42 insertions(+), 33 deletions(-) diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go index 0695ab37b..563a0921d 100644 --- a/server/controllers/events/events_controller_e2e_test.go +++ b/server/controllers/events/events_controller_e2e_test.go @@ -756,7 +756,7 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) { // Setup test dependencies. w := httptest.NewRecorder() - When(vcsClient.PullIsMergeable(AnyRepo(), matchers.AnyModelsPullRequest())).ThenReturn(true, nil) + When(vcsClient.PullIsMergeable(AnyRepo(), matchers.AnyModelsPullRequest(), "atlantis-test")).ThenReturn(true, nil) When(vcsClient.PullIsApproved(AnyRepo(), matchers.AnyModelsPullRequest())).ThenReturn(models.ApprovalStatus{ IsApproved: true, }, nil) @@ -1045,6 +1045,7 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl parallelPoolSize, silenceNoProjects, false, + "atlantis-test", e2ePullReqStatusFetcher, ) diff --git a/server/events/apply_command_runner.go b/server/events/apply_command_runner.go index db8e27c16..d6b6832ce 100644 --- a/server/events/apply_command_runner.go +++ b/server/events/apply_command_runner.go @@ -22,6 +22,7 @@ func NewApplyCommandRunner( parallelPoolSize int, SilenceNoProjects bool, silenceVCSStatusNoProjects bool, + VCSStatusName string, pullReqStatusFetcher vcs.PullReqStatusFetcher, ) *ApplyCommandRunner { return &ApplyCommandRunner{ @@ -38,6 +39,7 @@ func NewApplyCommandRunner( parallelPoolSize: parallelPoolSize, SilenceNoProjects: SilenceNoProjects, silenceVCSStatusNoProjects: silenceVCSStatusNoProjects, + VCSStatusName: VCSStatusName, pullReqStatusFetcher: pullReqStatusFetcher, } } @@ -60,6 +62,7 @@ type ApplyCommandRunner struct { SilenceNoProjects bool // SilenceVCSStatusNoPlans is whether any plan should set commit status if no projects // are found + VCSStatusName string silenceVCSStatusNoProjects bool } @@ -103,7 +106,7 @@ func (a *ApplyCommandRunner) Run(ctx *command.Context, cmd *CommentCommand) { // required the Atlantis status checks to pass, then we've now changed // the mergeability status of the pull request. // This sets the approved, mergeable, and sqlocked status in the context. - ctx.PullRequestStatus, err = a.pullReqStatusFetcher.FetchPullStatus(baseRepo, pull) + ctx.PullRequestStatus, err = a.pullReqStatusFetcher.FetchPullStatus(baseRepo, pull, a.VCSStatusName) if err != nil { // On error we continue the request with mergeable assumed false. // We want to continue because not all apply's will need this status, diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go index 5f203e9be..1e1403836 100644 --- a/server/events/command_runner_test.go +++ b/server/events/command_runner_test.go @@ -107,6 +107,7 @@ func setup(t *testing.T) *vcsmocks.MockClient { parallelPoolSize := 1 SilenceNoProjects := false + StatusName := "atlantis-test" policyCheckCommandRunner = events.NewPolicyCheckCommandRunner( dbUpdater, pullUpdater, @@ -151,6 +152,7 @@ func setup(t *testing.T) *vcsmocks.MockClient { parallelPoolSize, SilenceNoProjects, false, + StatusName, pullReqStatusFetcher, ) @@ -781,7 +783,7 @@ func TestApplyMergeablityWhenPolicyCheckFails(t *testing.T) { }, }) - When(ch.VCSClient.PullIsMergeable(fixtures.GithubRepo, modelPull)).ThenReturn(true, nil) + When(ch.VCSClient.PullIsMergeable(fixtures.GithubRepo, modelPull, "atlantis-test")).ThenReturn(true, nil) When(projectCommandBuilder.BuildApplyCommands(matchers.AnyPtrToEventsCommandContext(), matchers.AnyPtrToEventsCommentCommand())).Then(func(args []Param) ReturnValues { return ReturnValues{ diff --git a/server/events/vcs/azuredevops_client.go b/server/events/vcs/azuredevops_client.go index 6d0b4469d..abc7039d3 100644 --- a/server/events/vcs/azuredevops_client.go +++ b/server/events/vcs/azuredevops_client.go @@ -167,7 +167,7 @@ func (g *AzureDevopsClient) PullIsApproved(repo models.Repo, pull models.PullReq } // PullIsMergeable returns true if the merge request can be merged. -func (g *AzureDevopsClient) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { +func (g *AzureDevopsClient) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) opts := azuredevops.PullRequestGetOptions{IncludeWorkItemRefs: true} diff --git a/server/events/vcs/azuredevops_client_test.go b/server/events/vcs/azuredevops_client_test.go index a9314fd03..f824d1cfd 100644 --- a/server/events/vcs/azuredevops_client_test.go +++ b/server/events/vcs/azuredevops_client_test.go @@ -414,7 +414,7 @@ func TestAzureDevopsClient_PullIsMergeable(t *testing.T) { }, }, models.PullRequest{ Num: 1, - }) + }, "atlantis-test") Ok(t, err) Equals(t, c.expMergeable, actMergeable) }) diff --git a/server/events/vcs/bitbucketcloud/client.go b/server/events/vcs/bitbucketcloud/client.go index 598bce5df..b40df730e 100644 --- a/server/events/vcs/bitbucketcloud/client.go +++ b/server/events/vcs/bitbucketcloud/client.go @@ -132,7 +132,7 @@ func (b *Client) PullIsApproved(repo models.Repo, pull models.PullRequest) (appr } // PullIsMergeable returns true if the merge request has no conflicts and can be merged. -func (b *Client) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { +func (b *Client) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { nextPageURL := fmt.Sprintf("%s/2.0/repositories/%s/pullrequests/%d/diffstat", b.BaseURL, repo.FullName, pull.Num) // We'll only loop 1000 times as a safety measure. maxLoops := 1000 diff --git a/server/events/vcs/bitbucketcloud/client_test.go b/server/events/vcs/bitbucketcloud/client_test.go index 54e5179be..a193dc474 100644 --- a/server/events/vcs/bitbucketcloud/client_test.go +++ b/server/events/vcs/bitbucketcloud/client_test.go @@ -337,7 +337,7 @@ func TestClient_PullIsMergeable(t *testing.T) { }, }, models.PullRequest{ Num: 1, - }) + }, "atlantis-test") Ok(t, err) Equals(t, c.ExpMergeable, actMergeable) }) diff --git a/server/events/vcs/bitbucketserver/client.go b/server/events/vcs/bitbucketserver/client.go index e0041e1d3..f1c1991e2 100644 --- a/server/events/vcs/bitbucketserver/client.go +++ b/server/events/vcs/bitbucketserver/client.go @@ -193,7 +193,7 @@ func (b *Client) PullIsApproved(repo models.Repo, pull models.PullRequest) (appr } // PullIsMergeable returns true if the merge request has no conflicts and can be merged. -func (b *Client) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { +func (b *Client) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { projectKey, err := b.GetProjectKey(repo.Name, repo.SanitizedCloneURL) if err != nil { return false, err diff --git a/server/events/vcs/client.go b/server/events/vcs/client.go index e63fdcd1f..5d932cb56 100644 --- a/server/events/vcs/client.go +++ b/server/events/vcs/client.go @@ -27,7 +27,7 @@ type Client interface { CreateComment(repo models.Repo, pullNum int, comment string, command string) error HidePrevCommandComments(repo models.Repo, pullNum int, command string) error PullIsApproved(repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error) - PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) + PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) // UpdateStatus updates the commit status to state for pull. src is the // source of this status. This should be relatively static across runs, // ex. atlantis/plan or atlantis/apply. diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 74e56b304..6837115bc 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -298,7 +298,7 @@ func (g *GithubClient) PullIsApproved(repo models.Repo, pull models.PullRequest) } // PullIsMergeable returns true if the pull request is mergeable. -func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { +func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { githubPR, err := g.GetPullRequest(repo, pull.Num) if err != nil { return false, errors.Wrap(err, "getting pull request") @@ -339,8 +339,8 @@ func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest } for _, status := range allStatuses { for _, requiredCheck := range requiredChecks.Contexts { - // Ignore any commit statuses with 'altantis/apply' as prefix - if strings.HasPrefix(status.GetContext(), fmt.Sprintf("atlantis/%s", command.Apply.String())) { + // Ignore any commit statuses with 'atlantis/apply' as prefix + if strings.HasPrefix(status.GetContext(), fmt.Sprintf("%s/%s", vcsstatusname, command.Apply.String())) { continue } if status.GetContext() == requiredCheck { diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index 72f887f67..c2b7b7e81 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -475,6 +475,7 @@ func TestGithubClient_PullIsApproved(t *testing.T) { } func TestGithubClient_PullIsMergeable(t *testing.T) { + vcsStatusName := "atlantis-test" cases := []struct { state string requiredCheckName string @@ -531,13 +532,13 @@ func TestGithubClient_PullIsMergeable(t *testing.T) { }, { "blocked", - "atlantis/apply", + fmt.Sprintf("%s/apply", vcsStatusName), "failure", true, }, { "blocked", - "atlantis/apply", + fmt.Sprintf("%s/apply", vcsStatusName), "pending", true, }, @@ -611,7 +612,7 @@ func TestGithubClient_PullIsMergeable(t *testing.T) { Num: 1, HeadBranch: "headBranch", BaseBranch: "baseBranch", - }) + }, vcsStatusName) Ok(t, err) Equals(t, c.expMergeable, actMergeable) }) diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 6f35b88c2..767d02b7d 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -193,7 +193,7 @@ func (g *GitlabClient) PullIsApproved(repo models.Repo, pull models.PullRequest) // See: // - https://gitlab.com/gitlab-org/gitlab-ee/issues/3169 // - https://gitlab.com/gitlab-org/gitlab-ce/issues/42344 -func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { +func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { mr, _, err := g.Client.MergeRequests.GetMergeRequest(repo.FullName, pull.Num, nil) if err != nil { return false, err @@ -212,8 +212,8 @@ func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest } for _, status := range statuses { - // Ignore any commit statuses with 'altantis/apply' as prefix - if strings.HasPrefix(status.Name, fmt.Sprintf("atlantis/%s", command.Apply.String())) { + // Ignore any commit statuses with 'atlantis/apply' as prefix + if strings.HasPrefix(status.Name, fmt.Sprintf("%s/%s", vcsstatusname, command.Apply.String())) { continue } if !status.AllowFailure && project.OnlyAllowMergeIfPipelineSucceeds && status.Status != "success" { diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index a49634ebc..cd16009d0 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -250,33 +250,34 @@ func TestGitlabClient_UpdateStatus(t *testing.T) { func TestGitlabClient_PullIsMergeable(t *testing.T) { gitlabClientUnderTest = true + vcsStatusName := "atlantis-test" cases := []struct { statusName string status models.CommitStatus expState bool }{ { - "atlantis/apply: resource/default", + fmt.Sprintf("%s/apply: resource/default", vcsStatusName), models.FailedCommitStatus, true, }, { - "atlantis/apply", + fmt.Sprintf("%s/apply", vcsStatusName), models.FailedCommitStatus, true, }, { - "atlantis/plan: resource/default", + fmt.Sprintf("%s/plan: resource/default", vcsStatusName), models.FailedCommitStatus, false, }, { - "atlantis/plan", + fmt.Sprintf("%s/plan", vcsStatusName), models.PendingCommitStatus, false, }, { - "atlantis/plan", + fmt.Sprintf("%s/plan", vcsStatusName), models.SuccessCommitStatus, true, }, @@ -325,7 +326,7 @@ func TestGitlabClient_PullIsMergeable(t *testing.T) { Num: 1, BaseRepo: repo, HeadCommit: "sha", - }) + }, vcsStatusName) Ok(t, err) Equals(t, c.expState, mergeable) }) diff --git a/server/events/vcs/instrumented_client.go b/server/events/vcs/instrumented_client.go index 18d5025b9..d52d13bc8 100644 --- a/server/events/vcs/instrumented_client.go +++ b/server/events/vcs/instrumented_client.go @@ -166,7 +166,7 @@ func (c *InstrumentedClient) PullIsApproved(repo models.Repo, pull models.PullRe return approved, err } -func (c *InstrumentedClient) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { +func (c *InstrumentedClient) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { scope := c.StatsScope.SubScope("pull_is_mergeable") logger := c.Logger.WithHistory(fmtLogSrc(repo, pull.Num)...) @@ -176,7 +176,7 @@ func (c *InstrumentedClient) PullIsMergeable(repo models.Repo, pull models.PullR executionSuccess := scope.Counter(metrics.ExecutionSuccessMetric) executionError := scope.Counter(metrics.ExecutionErrorMetric) - mergeable, err := c.Client.PullIsMergeable(repo, pull) + mergeable, err := c.Client.PullIsMergeable(repo, pull, vcsstatusname) if err != nil { executionError.Inc(1) diff --git a/server/events/vcs/mocks/mock_client.go b/server/events/vcs/mocks/mock_client.go index 319949ded..0f7000857 100644 --- a/server/events/vcs/mocks/mock_client.go +++ b/server/events/vcs/mocks/mock_client.go @@ -170,7 +170,7 @@ func (mock *MockClient) GetTeamNamesForUser(repo models.Repo, user models.User) return ret0, ret1 } -func (mock *MockClient) PullIsMergeable(_param0 models.Repo, _param1 models.PullRequest) (bool, error) { +func (mock *MockClient) PullIsMergeable(_param0 models.Repo, _param1 models.PullRequest, vcsstatusname string) (bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockClient().") } diff --git a/server/events/vcs/not_configured_vcs_client.go b/server/events/vcs/not_configured_vcs_client.go index 45d441622..fca7a662b 100644 --- a/server/events/vcs/not_configured_vcs_client.go +++ b/server/events/vcs/not_configured_vcs_client.go @@ -38,7 +38,7 @@ func (a *NotConfiguredVCSClient) HidePrevCommandComments(repo models.Repo, pullN func (a *NotConfiguredVCSClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error) { return models.ApprovalStatus{}, a.err() } -func (a *NotConfiguredVCSClient) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { +func (a *NotConfiguredVCSClient) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { return false, a.err() } func (a *NotConfiguredVCSClient) UpdateStatus(repo models.Repo, pull models.PullRequest, state models.CommitStatus, src string, description string, url string) error { diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index 2e937b4cf..0ffb50dcf 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -68,8 +68,8 @@ func (d *ClientProxy) PullIsApproved(repo models.Repo, pull models.PullRequest) return d.clients[repo.VCSHost.Type].PullIsApproved(repo, pull) } -func (d *ClientProxy) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) { - return d.clients[repo.VCSHost.Type].PullIsMergeable(repo, pull) +func (d *ClientProxy) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { + return d.clients[repo.VCSHost.Type].PullIsMergeable(repo, pull, vcsstatusname) } func (d *ClientProxy) UpdateStatus(repo models.Repo, pull models.PullRequest, state models.CommitStatus, src string, description string, url string) error { diff --git a/server/events/vcs/pull_status_fetcher.go b/server/events/vcs/pull_status_fetcher.go index 2cbb75266..e8a9c11d3 100644 --- a/server/events/vcs/pull_status_fetcher.go +++ b/server/events/vcs/pull_status_fetcher.go @@ -6,7 +6,7 @@ import ( ) type PullReqStatusFetcher interface { - FetchPullStatus(repo models.Repo, pull models.PullRequest) (models.PullReqStatus, error) + FetchPullStatus(repo models.Repo, pull models.PullRequest, vcsstatusname string) (models.PullReqStatus, error) } type pullReqStatusFetcher struct { @@ -19,13 +19,13 @@ func NewPullReqStatusFetcher(client Client) PullReqStatusFetcher { } } -func (f *pullReqStatusFetcher) FetchPullStatus(repo models.Repo, pull models.PullRequest) (pullStatus models.PullReqStatus, err error) { +func (f *pullReqStatusFetcher) FetchPullStatus(repo models.Repo, pull models.PullRequest, vcsstatusname string) (pullStatus models.PullReqStatus, err error) { approvalStatus, err := f.client.PullIsApproved(repo, pull) if err != nil { return pullStatus, errors.Wrapf(err, "fetching pull approval status for repo: %s, and pull number: %d", repo.FullName, pull.Num) } - mergeable, err := f.client.PullIsMergeable(repo, pull) + mergeable, err := f.client.PullIsMergeable(repo, pull, vcsstatusname) if err != nil { return pullStatus, errors.Wrapf(err, "fetching mergeability status for repo: %s, and pull number: %d", repo.FullName, pull.Num) } diff --git a/server/server.go b/server/server.go index df7699418..fa3641161 100644 --- a/server/server.go +++ b/server/server.go @@ -631,6 +631,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { userConfig.ParallelPoolSize, userConfig.SilenceNoProjects, userConfig.SilenceVCSStatusNoProjects, + userConfig.VCSStatusName, pullReqStatusFetcher, )