From 6e6cab6b52cf2138f1cdb307b1fee3c8075a60f7 Mon Sep 17 00:00:00 2001 From: Ben Apprederisse Date: Thu, 3 Oct 2024 20:05:04 -0700 Subject: [PATCH 01/10] feat: add other-vcs-status-names-to-ignore `other-vcs-status-names-to-ignore` is a comma-separated list of atlantis vcs status names that will be ignored when deciding if the PR is mergeable or not. This PR is a proposal to solve [#2848](https://github.com/runatlantis/atlantis/issues/2848) for github client. --- cmd/server.go | 9 ++++++ cmd/server_test.go | 1 + .../events/events_controller_e2e_test.go | 4 +-- server/events/command_runner_test.go | 2 +- server/events/vcs/azuredevops_client.go | 2 +- server/events/vcs/azuredevops_client_test.go | 2 +- server/events/vcs/bitbucketcloud/client.go | 2 +- .../events/vcs/bitbucketcloud/client_test.go | 2 +- server/events/vcs/bitbucketserver/client.go | 2 +- server/events/vcs/client.go | 2 +- server/events/vcs/gitea/client.go | 2 +- server/events/vcs/github_client.go | 29 ++++++++++++++----- server/events/vcs/github_client_test.go | 4 +-- server/events/vcs/gitlab_client.go | 2 +- server/events/vcs/gitlab_client_test.go | 2 +- server/events/vcs/instrumented_client.go | 4 +-- server/events/vcs/mocks/mock_client.go | 4 +-- .../events/vcs/not_configured_vcs_client.go | 2 +- server/events/vcs/proxy.go | 4 +-- server/events/vcs/pull_status_fetcher.go | 14 +++++---- server/server.go | 2 +- server/user_config.go | 1 + 22 files changed, 62 insertions(+), 36 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index e53ca20418..9725f29496 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -146,6 +146,7 @@ const ( UseTFPluginCache = "use-tf-plugin-cache" VarFileAllowlistFlag = "var-file-allowlist" VCSStatusName = "vcs-status-name" + OtherVCSStatusNamesToIgnore = "other-vcs-status-names-to-ignore" TFEHostnameFlag = "tfe-hostname" TFELocalExecutionModeFlag = "tfe-local-execution-mode" TFETokenFlag = "tfe-token" @@ -175,6 +176,7 @@ const ( DefaultGitlabHostname = "gitlab.com" DefaultLockingDBType = "boltdb" DefaultLogLevel = "info" + DefaultOtherVCSStatusNamesToIgnore = "" DefaultMaxCommentsPerCommand = 100 DefaultParallelPoolSize = 15 DefaultStatsNamespace = "atlantis" @@ -439,6 +441,10 @@ var stringFlags = map[string]stringFlag{ description: "Comma-separated list of additional paths where variable definition files can be read from." + " If this argument is not provided, it defaults to Atlantis' data directory, determined by the --data-dir argument.", }, + OtherVCSStatusNamesToIgnore: { + description: "Comma-separated list of other Atlantis servers to ignore for pull request statuses.", + defaultValue: DefaultOtherVCSStatusNamesToIgnore, + }, VCSStatusName: { description: "Name used to identify Atlantis for pull request statuses.", defaultValue: DefaultVCSStatusName, @@ -918,6 +924,9 @@ func (s *ServerCmd) setDefaults(c *server.UserConfig, v *viper.Viper) { if c.VCSStatusName == "" { c.VCSStatusName = DefaultVCSStatusName } + if c.OtherVCSStatusNamesToIgnore == "" { + c.OtherVCSStatusNamesToIgnore = DefaultOtherVCSStatusNamesToIgnore + } if c.TFEHostname == "" { c.TFEHostname = DefaultTFEHostname } diff --git a/cmd/server_test.go b/cmd/server_test.go index cccf9cc055..8800f0a82a 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -145,6 +145,7 @@ var testFlags = map[string]interface{}{ UseTFPluginCache: true, VarFileAllowlistFlag: "/path", VCSStatusName: "my-status", + OtherVCSStatusNamesToIgnore: "", WebBasicAuthFlag: false, WebPasswordFlag: "atlantis", WebUsernameFlag: "atlantis", diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go index 68e517709c..4a9fefeffe 100644 --- a/server/controllers/events/events_controller_e2e_test.go +++ b/server/controllers/events/events_controller_e2e_test.go @@ -1191,7 +1191,7 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) { // Setup test dependencies. w := httptest.NewRecorder() When(vcsClient.PullIsMergeable( - Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Eq("atlantis-test"))).ThenReturn(true, nil) + Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Eq("atlantis-test"), []string{})).ThenReturn(true, nil) When(vcsClient.PullIsApproved( Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(models.ApprovalStatus{ IsApproved: true, @@ -1505,7 +1505,7 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers userConfig.QuietPolicyChecks, ) - e2ePullReqStatusFetcher := vcs.NewPullReqStatusFetcher(e2eVCSClient, "atlantis-test") + e2ePullReqStatusFetcher := vcs.NewPullReqStatusFetcher(e2eVCSClient, "atlantis-test", []string{}) planCommandRunner := events.NewPlanCommandRunner( false, diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go index 07026b6837..24293c6a89 100644 --- a/server/events/command_runner_test.go +++ b/server/events/command_runner_test.go @@ -1148,7 +1148,7 @@ func TestApplyMergeablityWhenPolicyCheckFails(t *testing.T) { }, }) - When(ch.VCSClient.PullIsMergeable(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(modelPull), Eq("atlantis-test"))).ThenReturn(true, nil) + When(ch.VCSClient.PullIsMergeable(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(modelPull), Eq("atlantis-test"), Eq([]string{}))).ThenReturn(true, nil) When(projectCommandBuilder.BuildApplyCommands(Any[*command.Context](), Any[*events.CommentCommand]())).Then(func(args []Param) ReturnValues { return ReturnValues{ diff --git a/server/events/vcs/azuredevops_client.go b/server/events/vcs/azuredevops_client.go index 1d115d28e9..63344d85e7 100644 --- a/server/events/vcs/azuredevops_client.go +++ b/server/events/vcs/azuredevops_client.go @@ -177,7 +177,7 @@ func (g *AzureDevopsClient) DiscardReviews(repo models.Repo, pull models.PullReq } // PullIsMergeable returns true if the merge request can be merged. -func (g *AzureDevopsClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { //nolint: revive +func (g *AzureDevopsClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, _ string, _ []string) (bool, error) { //nolint: revive 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 1e428023b8..a7095262d2 100644 --- a/server/events/vcs/azuredevops_client_test.go +++ b/server/events/vcs/azuredevops_client_test.go @@ -428,7 +428,7 @@ func TestAzureDevopsClient_PullIsMergeable(t *testing.T) { }, }, models.PullRequest{ Num: 1, - }, "atlantis-test") + }, "atlantis-test", []string{}) 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 a0dcfd7161..c9a88f0245 100644 --- a/server/events/vcs/bitbucketcloud/client.go +++ b/server/events/vcs/bitbucketcloud/client.go @@ -139,7 +139,7 @@ func (b *Client) PullIsApproved(logger logging.SimpleLogging, repo models.Repo, } // PullIsMergeable returns true if the merge request has no conflicts and can be merged. -func (b *Client) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, _ string) (bool, error) { +func (b *Client) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, _ string, _ []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 0a4bd48db6..59108a14f3 100644 --- a/server/events/vcs/bitbucketcloud/client_test.go +++ b/server/events/vcs/bitbucketcloud/client_test.go @@ -352,7 +352,7 @@ func TestClient_PullIsMergeable(t *testing.T) { }, }, models.PullRequest{ Num: 1, - }, "atlantis-test") + }, "atlantis-test", []string{}) 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 69dbbde518..83956db629 100644 --- a/server/events/vcs/bitbucketserver/client.go +++ b/server/events/vcs/bitbucketserver/client.go @@ -203,7 +203,7 @@ func (b *Client) DiscardReviews(_ models.Repo, _ models.PullRequest) error { } // PullIsMergeable returns true if the merge request has no conflicts and can be merged. -func (b *Client) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, _ string) (bool, error) { +func (b *Client) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, _ string, _ []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 b6ad7cb9cd..dfc058b696 100644 --- a/server/events/vcs/client.go +++ b/server/events/vcs/client.go @@ -30,7 +30,7 @@ type Client interface { ReactToComment(logger logging.SimpleLogging, repo models.Repo, pullNum int, commentID int64, reaction string) error HidePrevCommandComments(logger logging.SimpleLogging, repo models.Repo, pullNum int, command string, dir string) error PullIsApproved(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error) - PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) + PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []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/gitea/client.go b/server/events/vcs/gitea/client.go index f9deb2cb74..9867d4c8f9 100644 --- a/server/events/vcs/gitea/client.go +++ b/server/events/vcs/gitea/client.go @@ -283,7 +283,7 @@ func (c *GiteaClient) PullIsApproved(logger logging.SimpleLogging, repo models.R } // PullIsMergeable returns true if the pull request is mergeable -func (c *GiteaClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { +func (c *GiteaClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, _ string, _ []string) (bool, error) { logger.Debug("Checking if Gitea pull request %d is mergeable", pull.Num) pullRequest, _, err := c.giteaClient.GetPullRequest(repo.Owner, repo.Name, int64(pull.Num)) diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 1129867064..abf2ac2446 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -709,12 +709,25 @@ func CheckRunPassed(checkRun CheckRun) bool { return checkRun.Conclusion == "SUCCESS" || checkRun.Conclusion == "SKIPPED" || checkRun.Conclusion == "NEUTRAL" } -func StatusContextPassed(statusContext StatusContext, vcsstatusname string) bool { +func StatusContextPassed(statusContext StatusContext, vcsstatusname string, otherStatusNamesToIgnore []string) bool { + // iterates through the list of other status names that were set to be ignored, allowing other atlantis servers + // to not be considered when determining if the status context is successful + otherStatusNamesOK := false + if len(otherStatusNamesToIgnore) > 0 { + otherStatusNamesOK = true + } + for _, otherStatusNameToIgnore := range otherStatusNamesToIgnore { + if !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", otherStatusNameToIgnore, command.Plan.String())) || + !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", otherStatusNameToIgnore, command.Apply.String())) { + otherStatusNamesOK = false + break + } + } return strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", vcsstatusname, command.Apply.String())) || - statusContext.State == "SUCCESS" + statusContext.State == "SUCCESS" || otherStatusNamesOK } -func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, statusContexts []StatusContext, vcsstatusname string) bool { +func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, statusContexts []StatusContext, vcsstatusname string, otherStatusNamesToIgnore []string) bool { for _, checkRun := range checkRuns { if checkRun.Name == expectedContext { return CheckRunPassed(checkRun) @@ -723,7 +736,7 @@ func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, for _, statusContext := range statusContexts { if statusContext.Context == expectedContext { - return StatusContextPassed(statusContext, vcsstatusname) + return StatusContextPassed(statusContext, vcsstatusname, otherStatusNamesToIgnore) } } @@ -748,7 +761,7 @@ func (g *GithubClient) ExpectedWorkflowPassed(expectedWorkflow WorkflowFileRefer } // IsMergeableMinusApply checks review decision (which takes into account CODEOWNERS) and required checks for PR (excluding the atlantis apply check). -func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo models.Repo, pull *github.PullRequest, vcsstatusname string) (bool, error) { +func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo models.Repo, pull *github.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { if pull.Number == nil { return false, errors.New("pull request number is nil") } @@ -771,7 +784,7 @@ func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo // Go through all checks and workflows required by branch protection or rulesets // Make sure that they can all be found in the statusCheckRollup and that they all pass for _, requiredCheck := range requiredChecks { - if !ExpectedCheckPassed(requiredCheck, checkRuns, statusContexts, vcsstatusname) { + if !ExpectedCheckPassed(requiredCheck, checkRuns, statusContexts, vcsstatusname, otherStatusNamesToIgnore) { logger.Debug("%s: Expected Required Check: %s", notMergeablePrefix, requiredCheck) return false, nil } @@ -791,7 +804,7 @@ func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo } // PullIsMergeable returns true if the pull request is mergeable. -func (g *GithubClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { +func (g *GithubClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { logger.Debug("Checking if GitHub pull request %d is mergeable", pull.Num) githubPR, err := g.GetPullRequest(logger, repo, pull.Num) if err != nil { @@ -813,7 +826,7 @@ func (g *GithubClient) PullIsMergeable(logger logging.SimpleLogging, repo models case "blocked": if g.config.AllowMergeableBypassApply { logger.Debug("AllowMergeableBypassApply feature flag is enabled - attempting to bypass apply from mergeable requirements") - isMergeableMinusApply, err := g.IsMergeableMinusApply(logger, repo, githubPR, vcsstatusname) + isMergeableMinusApply, err := g.IsMergeableMinusApply(logger, repo, githubPR, vcsstatusname, otherStatusNamesToIgnore) if err != nil { return false, errors.Wrap(err, "getting pull request status") } diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index ef97c64245..ed8a57fbc3 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -609,7 +609,7 @@ func TestGithubClient_PullIsMergeable(t *testing.T) { }, }, models.PullRequest{ Num: 1, - }, vcsStatusName) + }, vcsStatusName, []string{}) Ok(t, err) Equals(t, c.expMergeable, actMergeable) }) @@ -873,7 +873,7 @@ func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) }, }, models.PullRequest{ Num: 1, - }, vcsStatusName) + }, vcsStatusName, []string{}) 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 289b1a0d8a..b78c82b271 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -300,7 +300,7 @@ func (g *GitlabClient) PullIsApproved(logger logging.SimpleLogging, repo models. // See: // - https://gitlab.com/gitlab-org/gitlab-ee/issues/3169 // - https://gitlab.com/gitlab-org/gitlab-ce/issues/42344 -func (g *GitlabClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { +func (g *GitlabClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, _ []string) (bool, error) { logger.Debug("Checking if GitLab merge request %d is mergeable", pull.Num) mr, resp, err := g.Client.MergeRequests.GetMergeRequest(repo.FullName, pull.Num, nil) if resp != nil { diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index cef424bcb0..98de58287f 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -659,7 +659,7 @@ func TestGitlabClient_PullIsMergeable(t *testing.T) { Num: c.mrID, BaseRepo: repo, HeadCommit: "67cb91d3f6198189f433c045154a885784ba6977", - }, vcsStatusName) + }, vcsStatusName, []string{}) 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 54e977e595..f697d38e54 100644 --- a/server/events/vcs/instrumented_client.go +++ b/server/events/vcs/instrumented_client.go @@ -183,7 +183,7 @@ func (c *InstrumentedClient) PullIsApproved(logger logging.SimpleLogging, repo m return approved, err } -func (c *InstrumentedClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { +func (c *InstrumentedClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { scope := c.StatsScope.SubScope("pull_is_mergeable") scope = SetGitScopeTags(scope, repo.FullName, pull.Num) @@ -193,7 +193,7 @@ func (c *InstrumentedClient) PullIsMergeable(logger logging.SimpleLogging, repo executionSuccess := scope.Counter(metrics.ExecutionSuccessMetric) executionError := scope.Counter(metrics.ExecutionErrorMetric) - mergeable, err := c.Client.PullIsMergeable(logger, repo, pull, vcsstatusname) + mergeable, err := c.Client.PullIsMergeable(logger, repo, pull, vcsstatusname, otherStatusNamesToIgnore) 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 ffa37fe8cb..9a8e0b2fee 100644 --- a/server/events/vcs/mocks/mock_client.go +++ b/server/events/vcs/mocks/mock_client.go @@ -223,11 +223,11 @@ func (mock *MockClient) PullIsApproved(logger logging.SimpleLogging, repo models return ret0, ret1 } -func (mock *MockClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { +func (mock *MockClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockClient().") } - params := []pegomock.Param{logger, repo, pull, vcsstatusname} + params := []pegomock.Param{logger, repo, pull, vcsstatusname, otherStatusNamesToIgnore} result := pegomock.GetGenericMockFrom(mock).Invoke("PullIsMergeable", params, []reflect.Type{reflect.TypeOf((*bool)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) var ret0 bool var ret1 error diff --git a/server/events/vcs/not_configured_vcs_client.go b/server/events/vcs/not_configured_vcs_client.go index 6fd7a731cb..41b14ad2c6 100644 --- a/server/events/vcs/not_configured_vcs_client.go +++ b/server/events/vcs/not_configured_vcs_client.go @@ -45,7 +45,7 @@ func (a *NotConfiguredVCSClient) PullIsApproved(_ logging.SimpleLogging, _ model func (a *NotConfiguredVCSClient) DiscardReviews(_ models.Repo, _ models.PullRequest) error { return nil } -func (a *NotConfiguredVCSClient) PullIsMergeable(_ logging.SimpleLogging, _ models.Repo, _ models.PullRequest, _ string) (bool, error) { +func (a *NotConfiguredVCSClient) PullIsMergeable(_ logging.SimpleLogging, _ models.Repo, _ models.PullRequest, _ string, _ []string) (bool, error) { return false, a.err() } func (a *NotConfiguredVCSClient) UpdateStatus(_ logging.SimpleLogging, _ models.Repo, _ models.PullRequest, _ models.CommitStatus, _ string, _ string, _ string) error { diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index cd67b84c90..b2f73a86e8 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -81,8 +81,8 @@ func (d *ClientProxy) DiscardReviews(repo models.Repo, pull models.PullRequest) return d.clients[repo.VCSHost.Type].DiscardReviews(repo, pull) } -func (d *ClientProxy) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) { - return d.clients[repo.VCSHost.Type].PullIsMergeable(logger, repo, pull, vcsstatusname) +func (d *ClientProxy) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { + return d.clients[repo.VCSHost.Type].PullIsMergeable(logger, repo, pull, vcsstatusname, otherStatusNamesToIgnore) } func (d *ClientProxy) UpdateStatus(logger logging.SimpleLogging, 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 b5b8a46383..514be2b5b5 100644 --- a/server/events/vcs/pull_status_fetcher.go +++ b/server/events/vcs/pull_status_fetcher.go @@ -13,14 +13,16 @@ type PullReqStatusFetcher interface { } type pullReqStatusFetcher struct { - client Client - vcsStatusName string + client Client + vcsStatusName string + otherStatusNamesToIgnore []string } -func NewPullReqStatusFetcher(client Client, vcsStatusName string) PullReqStatusFetcher { +func NewPullReqStatusFetcher(client Client, vcsStatusName string, otherStatusNamesToIgnore []string) PullReqStatusFetcher { return &pullReqStatusFetcher{ - client: client, - vcsStatusName: vcsStatusName, + client: client, + vcsStatusName: vcsStatusName, + otherStatusNamesToIgnore: otherStatusNamesToIgnore, } } @@ -30,7 +32,7 @@ func (f *pullReqStatusFetcher) FetchPullStatus(logger logging.SimpleLogging, pul return pullStatus, errors.Wrapf(err, "fetching pull approval status for repo: %s, and pull number: %d", pull.BaseRepo.FullName, pull.Num) } - mergeable, err := f.client.PullIsMergeable(logger, pull.BaseRepo, pull, f.vcsStatusName) + mergeable, err := f.client.PullIsMergeable(logger, pull.BaseRepo, pull, f.vcsStatusName, f.otherStatusNamesToIgnore) if err != nil { return pullStatus, errors.Wrapf(err, "fetching mergeability status for repo: %s, and pull number: %d", pull.BaseRepo.FullName, pull.Num) } diff --git a/server/server.go b/server/server.go index af940f3787..53104b4f93 100644 --- a/server/server.go +++ b/server/server.go @@ -722,7 +722,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { userConfig.QuietPolicyChecks, ) - pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient, userConfig.VCSStatusName) + pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient, userConfig.VCSStatusName, strings.Split(userConfig.OtherVCSStatusNamesToIgnore, ",")) planCommandRunner := events.NewPlanCommandRunner( userConfig.SilenceVCSStatusNoPlans, userConfig.SilenceVCSStatusNoProjects, diff --git a/server/user_config.go b/server/user_config.go index 9a6247ae09..79ec4675f8 100644 --- a/server/user_config.go +++ b/server/user_config.go @@ -75,6 +75,7 @@ type UserConfig struct { LogLevel string `mapstructure:"log-level"` MarkdownTemplateOverridesDir string `mapstructure:"markdown-template-overrides-dir"` MaxCommentsPerCommand int `mapstructure:"max-comments-per-command"` + OtherVCSStatusNamesToIgnore string `mapstructure:"other-vcs-status-names-to-ignore"` ParallelPoolSize int `mapstructure:"parallel-pool-size"` ParallelPlan bool `mapstructure:"parallel-plan"` ParallelApply bool `mapstructure:"parallel-apply"` From 102f42dc51cd52aa608debd210f5f7902c900a87 Mon Sep 17 00:00:00 2001 From: Ben Apprederisse Date: Thu, 3 Oct 2024 20:21:34 -0700 Subject: [PATCH 02/10] fix: TestGitHubWorkflowWithPolicyCheck/1_failing_policy_and_1_passing_policy_ --- server/controllers/events/events_controller_e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go index 4a9fefeffe..e433372b0d 100644 --- a/server/controllers/events/events_controller_e2e_test.go +++ b/server/controllers/events/events_controller_e2e_test.go @@ -1191,7 +1191,7 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) { // Setup test dependencies. w := httptest.NewRecorder() When(vcsClient.PullIsMergeable( - Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Eq("atlantis-test"), []string{})).ThenReturn(true, nil) + Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Eq("atlantis-test"), Eq([]string{}))).ThenReturn(true, nil) When(vcsClient.PullIsApproved( Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(models.ApprovalStatus{ IsApproved: true, From 69ea14f3fa7da1a3c65c66b57625c40aa3cb7578 Mon Sep 17 00:00:00 2001 From: Ben Apprederisse Date: Fri, 4 Oct 2024 10:09:46 -0700 Subject: [PATCH 03/10] fix: rename flag to `ignore-vcs-status-names` per suggestion --- cmd/server.go | 12 ++++++------ cmd/server_test.go | 2 +- server/events/vcs/client.go | 2 +- server/events/vcs/github_client.go | 22 +++++++++++----------- server/events/vcs/instrumented_client.go | 4 ++-- server/events/vcs/mocks/mock_client.go | 4 ++-- server/events/vcs/proxy.go | 4 ++-- server/events/vcs/pull_status_fetcher.go | 16 ++++++++-------- server/server.go | 2 +- server/user_config.go | 2 +- 10 files changed, 35 insertions(+), 35 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index 9725f29496..9a798417b2 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -146,7 +146,7 @@ const ( UseTFPluginCache = "use-tf-plugin-cache" VarFileAllowlistFlag = "var-file-allowlist" VCSStatusName = "vcs-status-name" - OtherVCSStatusNamesToIgnore = "other-vcs-status-names-to-ignore" + IgnoreVCSStatusNames = "ignore-vcs-status-names" TFEHostnameFlag = "tfe-hostname" TFELocalExecutionModeFlag = "tfe-local-execution-mode" TFETokenFlag = "tfe-token" @@ -176,7 +176,7 @@ const ( DefaultGitlabHostname = "gitlab.com" DefaultLockingDBType = "boltdb" DefaultLogLevel = "info" - DefaultOtherVCSStatusNamesToIgnore = "" + DefaultIgnoreVCSStatusNames = "" DefaultMaxCommentsPerCommand = 100 DefaultParallelPoolSize = 15 DefaultStatsNamespace = "atlantis" @@ -441,9 +441,9 @@ var stringFlags = map[string]stringFlag{ description: "Comma-separated list of additional paths where variable definition files can be read from." + " If this argument is not provided, it defaults to Atlantis' data directory, determined by the --data-dir argument.", }, - OtherVCSStatusNamesToIgnore: { + IgnoreVCSStatusNames: { description: "Comma-separated list of other Atlantis servers to ignore for pull request statuses.", - defaultValue: DefaultOtherVCSStatusNamesToIgnore, + defaultValue: DefaultIgnoreVCSStatusNames, }, VCSStatusName: { description: "Name used to identify Atlantis for pull request statuses.", @@ -924,8 +924,8 @@ func (s *ServerCmd) setDefaults(c *server.UserConfig, v *viper.Viper) { if c.VCSStatusName == "" { c.VCSStatusName = DefaultVCSStatusName } - if c.OtherVCSStatusNamesToIgnore == "" { - c.OtherVCSStatusNamesToIgnore = DefaultOtherVCSStatusNamesToIgnore + if c.IgnoreVCSStatusNames == "" { + c.IgnoreVCSStatusNames = DefaultIgnoreVCSStatusNames } if c.TFEHostname == "" { c.TFEHostname = DefaultTFEHostname diff --git a/cmd/server_test.go b/cmd/server_test.go index 8800f0a82a..2c9034af48 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -145,7 +145,7 @@ var testFlags = map[string]interface{}{ UseTFPluginCache: true, VarFileAllowlistFlag: "/path", VCSStatusName: "my-status", - OtherVCSStatusNamesToIgnore: "", + IgnoreVCSStatusNames: "", WebBasicAuthFlag: false, WebPasswordFlag: "atlantis", WebUsernameFlag: "atlantis", diff --git a/server/events/vcs/client.go b/server/events/vcs/client.go index dfc058b696..9e32981a82 100644 --- a/server/events/vcs/client.go +++ b/server/events/vcs/client.go @@ -30,7 +30,7 @@ type Client interface { ReactToComment(logger logging.SimpleLogging, repo models.Repo, pullNum int, commentID int64, reaction string) error HidePrevCommandComments(logger logging.SimpleLogging, repo models.Repo, pullNum int, command string, dir string) error PullIsApproved(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error) - PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) + PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, ignoreVCSStatusNames []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 abf2ac2446..48699c5b41 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -709,16 +709,16 @@ func CheckRunPassed(checkRun CheckRun) bool { return checkRun.Conclusion == "SUCCESS" || checkRun.Conclusion == "SKIPPED" || checkRun.Conclusion == "NEUTRAL" } -func StatusContextPassed(statusContext StatusContext, vcsstatusname string, otherStatusNamesToIgnore []string) bool { +func StatusContextPassed(statusContext StatusContext, vcsstatusname string, ignoreVCSStatusNames []string) bool { // iterates through the list of other status names that were set to be ignored, allowing other atlantis servers // to not be considered when determining if the status context is successful otherStatusNamesOK := false - if len(otherStatusNamesToIgnore) > 0 { + if len(ignoreVCSStatusNames) > 0 { otherStatusNamesOK = true } - for _, otherStatusNameToIgnore := range otherStatusNamesToIgnore { - if !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", otherStatusNameToIgnore, command.Plan.String())) || - !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", otherStatusNameToIgnore, command.Apply.String())) { + for _, ignoreVCSStatusName := range ignoreVCSStatusNames { + if !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", ignoreVCSStatusName, command.Plan.String())) || + !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", ignoreVCSStatusName, command.Apply.String())) { otherStatusNamesOK = false break } @@ -727,7 +727,7 @@ func StatusContextPassed(statusContext StatusContext, vcsstatusname string, othe statusContext.State == "SUCCESS" || otherStatusNamesOK } -func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, statusContexts []StatusContext, vcsstatusname string, otherStatusNamesToIgnore []string) bool { +func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, statusContexts []StatusContext, vcsstatusname string, ignoreVCSStatusNames []string) bool { for _, checkRun := range checkRuns { if checkRun.Name == expectedContext { return CheckRunPassed(checkRun) @@ -736,7 +736,7 @@ func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, for _, statusContext := range statusContexts { if statusContext.Context == expectedContext { - return StatusContextPassed(statusContext, vcsstatusname, otherStatusNamesToIgnore) + return StatusContextPassed(statusContext, vcsstatusname, ignoreVCSStatusNames) } } @@ -761,7 +761,7 @@ func (g *GithubClient) ExpectedWorkflowPassed(expectedWorkflow WorkflowFileRefer } // IsMergeableMinusApply checks review decision (which takes into account CODEOWNERS) and required checks for PR (excluding the atlantis apply check). -func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo models.Repo, pull *github.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { +func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo models.Repo, pull *github.PullRequest, vcsstatusname string, ignoreVCSStatusNames []string) (bool, error) { if pull.Number == nil { return false, errors.New("pull request number is nil") } @@ -784,7 +784,7 @@ func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo // Go through all checks and workflows required by branch protection or rulesets // Make sure that they can all be found in the statusCheckRollup and that they all pass for _, requiredCheck := range requiredChecks { - if !ExpectedCheckPassed(requiredCheck, checkRuns, statusContexts, vcsstatusname, otherStatusNamesToIgnore) { + if !ExpectedCheckPassed(requiredCheck, checkRuns, statusContexts, vcsstatusname, ignoreVCSStatusNames) { logger.Debug("%s: Expected Required Check: %s", notMergeablePrefix, requiredCheck) return false, nil } @@ -804,7 +804,7 @@ func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo } // PullIsMergeable returns true if the pull request is mergeable. -func (g *GithubClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { +func (g *GithubClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, ignoreVCSStatusNames []string) (bool, error) { logger.Debug("Checking if GitHub pull request %d is mergeable", pull.Num) githubPR, err := g.GetPullRequest(logger, repo, pull.Num) if err != nil { @@ -826,7 +826,7 @@ func (g *GithubClient) PullIsMergeable(logger logging.SimpleLogging, repo models case "blocked": if g.config.AllowMergeableBypassApply { logger.Debug("AllowMergeableBypassApply feature flag is enabled - attempting to bypass apply from mergeable requirements") - isMergeableMinusApply, err := g.IsMergeableMinusApply(logger, repo, githubPR, vcsstatusname, otherStatusNamesToIgnore) + isMergeableMinusApply, err := g.IsMergeableMinusApply(logger, repo, githubPR, vcsstatusname, ignoreVCSStatusNames) if err != nil { return false, errors.Wrap(err, "getting pull request status") } diff --git a/server/events/vcs/instrumented_client.go b/server/events/vcs/instrumented_client.go index f697d38e54..90eaeb5e7f 100644 --- a/server/events/vcs/instrumented_client.go +++ b/server/events/vcs/instrumented_client.go @@ -183,7 +183,7 @@ func (c *InstrumentedClient) PullIsApproved(logger logging.SimpleLogging, repo m return approved, err } -func (c *InstrumentedClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { +func (c *InstrumentedClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, ignoreVCSStatusNames []string) (bool, error) { scope := c.StatsScope.SubScope("pull_is_mergeable") scope = SetGitScopeTags(scope, repo.FullName, pull.Num) @@ -193,7 +193,7 @@ func (c *InstrumentedClient) PullIsMergeable(logger logging.SimpleLogging, repo executionSuccess := scope.Counter(metrics.ExecutionSuccessMetric) executionError := scope.Counter(metrics.ExecutionErrorMetric) - mergeable, err := c.Client.PullIsMergeable(logger, repo, pull, vcsstatusname, otherStatusNamesToIgnore) + mergeable, err := c.Client.PullIsMergeable(logger, repo, pull, vcsstatusname, ignoreVCSStatusNames) 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 9a8e0b2fee..f51036b87a 100644 --- a/server/events/vcs/mocks/mock_client.go +++ b/server/events/vcs/mocks/mock_client.go @@ -223,11 +223,11 @@ func (mock *MockClient) PullIsApproved(logger logging.SimpleLogging, repo models return ret0, ret1 } -func (mock *MockClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { +func (mock *MockClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, ignoreVCSStatusNames []string) (bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockClient().") } - params := []pegomock.Param{logger, repo, pull, vcsstatusname, otherStatusNamesToIgnore} + params := []pegomock.Param{logger, repo, pull, vcsstatusname, ignoreVCSStatusNames} result := pegomock.GetGenericMockFrom(mock).Invoke("PullIsMergeable", params, []reflect.Type{reflect.TypeOf((*bool)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) var ret0 bool var ret1 error diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index b2f73a86e8..68aa45bf58 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -81,8 +81,8 @@ func (d *ClientProxy) DiscardReviews(repo models.Repo, pull models.PullRequest) return d.clients[repo.VCSHost.Type].DiscardReviews(repo, pull) } -func (d *ClientProxy) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, otherStatusNamesToIgnore []string) (bool, error) { - return d.clients[repo.VCSHost.Type].PullIsMergeable(logger, repo, pull, vcsstatusname, otherStatusNamesToIgnore) +func (d *ClientProxy) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, ignoreVCSStatusNames []string) (bool, error) { + return d.clients[repo.VCSHost.Type].PullIsMergeable(logger, repo, pull, vcsstatusname, ignoreVCSStatusNames) } func (d *ClientProxy) UpdateStatus(logger logging.SimpleLogging, 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 514be2b5b5..b96d69011a 100644 --- a/server/events/vcs/pull_status_fetcher.go +++ b/server/events/vcs/pull_status_fetcher.go @@ -13,16 +13,16 @@ type PullReqStatusFetcher interface { } type pullReqStatusFetcher struct { - client Client - vcsStatusName string - otherStatusNamesToIgnore []string + client Client + vcsStatusName string + ignoreVCSStatusNames []string } -func NewPullReqStatusFetcher(client Client, vcsStatusName string, otherStatusNamesToIgnore []string) PullReqStatusFetcher { +func NewPullReqStatusFetcher(client Client, vcsStatusName string, ignoreVCSStatusNames []string) PullReqStatusFetcher { return &pullReqStatusFetcher{ - client: client, - vcsStatusName: vcsStatusName, - otherStatusNamesToIgnore: otherStatusNamesToIgnore, + client: client, + vcsStatusName: vcsStatusName, + ignoreVCSStatusNames: ignoreVCSStatusNames, } } @@ -32,7 +32,7 @@ func (f *pullReqStatusFetcher) FetchPullStatus(logger logging.SimpleLogging, pul return pullStatus, errors.Wrapf(err, "fetching pull approval status for repo: %s, and pull number: %d", pull.BaseRepo.FullName, pull.Num) } - mergeable, err := f.client.PullIsMergeable(logger, pull.BaseRepo, pull, f.vcsStatusName, f.otherStatusNamesToIgnore) + mergeable, err := f.client.PullIsMergeable(logger, pull.BaseRepo, pull, f.vcsStatusName, f.ignoreVCSStatusNames) if err != nil { return pullStatus, errors.Wrapf(err, "fetching mergeability status for repo: %s, and pull number: %d", pull.BaseRepo.FullName, pull.Num) } diff --git a/server/server.go b/server/server.go index 53104b4f93..f2d93b5a49 100644 --- a/server/server.go +++ b/server/server.go @@ -722,7 +722,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { userConfig.QuietPolicyChecks, ) - pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient, userConfig.VCSStatusName, strings.Split(userConfig.OtherVCSStatusNamesToIgnore, ",")) + pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient, userConfig.VCSStatusName, strings.Split(userConfig.IgnoreVCSStatusNames, ",")) planCommandRunner := events.NewPlanCommandRunner( userConfig.SilenceVCSStatusNoPlans, userConfig.SilenceVCSStatusNoProjects, diff --git a/server/user_config.go b/server/user_config.go index 79ec4675f8..8693a55549 100644 --- a/server/user_config.go +++ b/server/user_config.go @@ -75,7 +75,7 @@ type UserConfig struct { LogLevel string `mapstructure:"log-level"` MarkdownTemplateOverridesDir string `mapstructure:"markdown-template-overrides-dir"` MaxCommentsPerCommand int `mapstructure:"max-comments-per-command"` - OtherVCSStatusNamesToIgnore string `mapstructure:"other-vcs-status-names-to-ignore"` + IgnoreVCSStatusNames string `mapstructure:"ignore-vcs-status-names"` ParallelPoolSize int `mapstructure:"parallel-pool-size"` ParallelPlan bool `mapstructure:"parallel-plan"` ParallelApply bool `mapstructure:"parallel-apply"` From ca1171e0dd174edbbd63f8f6ae560caa6af4c0c7 Mon Sep 17 00:00:00 2001 From: Ben Apprederisse Date: Fri, 4 Oct 2024 13:42:22 -0700 Subject: [PATCH 04/10] fix: ignore ALL "ignore-vcs-status-names" checks --- server/events/vcs/github_client.go | 31 ++++++++++++------------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 48699c5b41..d39b98838f 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -18,6 +18,8 @@ import ( "encoding/base64" "fmt" "net/http" + + "slices" "strconv" "strings" "time" @@ -709,25 +711,12 @@ func CheckRunPassed(checkRun CheckRun) bool { return checkRun.Conclusion == "SUCCESS" || checkRun.Conclusion == "SKIPPED" || checkRun.Conclusion == "NEUTRAL" } -func StatusContextPassed(statusContext StatusContext, vcsstatusname string, ignoreVCSStatusNames []string) bool { - // iterates through the list of other status names that were set to be ignored, allowing other atlantis servers - // to not be considered when determining if the status context is successful - otherStatusNamesOK := false - if len(ignoreVCSStatusNames) > 0 { - otherStatusNamesOK = true - } - for _, ignoreVCSStatusName := range ignoreVCSStatusNames { - if !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", ignoreVCSStatusName, command.Plan.String())) || - !strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", ignoreVCSStatusName, command.Apply.String())) { - otherStatusNamesOK = false - break - } - } +func StatusContextPassed(statusContext StatusContext, vcsstatusname string) bool { return strings.HasPrefix(string(statusContext.Context), fmt.Sprintf("%s/%s", vcsstatusname, command.Apply.String())) || - statusContext.State == "SUCCESS" || otherStatusNamesOK + statusContext.State == "SUCCESS" } -func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, statusContexts []StatusContext, vcsstatusname string, ignoreVCSStatusNames []string) bool { +func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, statusContexts []StatusContext, vcsstatusname string) bool { for _, checkRun := range checkRuns { if checkRun.Name == expectedContext { return CheckRunPassed(checkRun) @@ -736,7 +725,7 @@ func ExpectedCheckPassed(expectedContext githubv4.String, checkRuns []CheckRun, for _, statusContext := range statusContexts { if statusContext.Context == expectedContext { - return StatusContextPassed(statusContext, vcsstatusname, ignoreVCSStatusNames) + return StatusContextPassed(statusContext, vcsstatusname) } } @@ -784,8 +773,8 @@ func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo // Go through all checks and workflows required by branch protection or rulesets // Make sure that they can all be found in the statusCheckRollup and that they all pass for _, requiredCheck := range requiredChecks { - if !ExpectedCheckPassed(requiredCheck, checkRuns, statusContexts, vcsstatusname, ignoreVCSStatusNames) { - logger.Debug("%s: Expected Required Check: %s", notMergeablePrefix, requiredCheck) + if !slices.Contains(ignoreVCSStatusNames, GetVCSStatusNameFromRequiredCheck(requiredCheck)) && !ExpectedCheckPassed(requiredCheck, checkRuns, statusContexts, vcsstatusname) { + logger.Debug("%s: Expected Required Check: %s VCS Status Name: %s Ignore VCS Status Names: %s", notMergeablePrefix, requiredCheck, vcsstatusname, ignoreVCSStatusNames) return false, nil } } @@ -803,6 +792,10 @@ func (g *GithubClient) IsMergeableMinusApply(logger logging.SimpleLogging, repo return true, nil } +func GetVCSStatusNameFromRequiredCheck(requiredCheck githubv4.String) string { + return strings.Split(string(requiredCheck), "/")[0] +} + // PullIsMergeable returns true if the pull request is mergeable. func (g *GithubClient) PullIsMergeable(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, vcsstatusname string, ignoreVCSStatusNames []string) (bool, error) { logger.Debug("Checking if GitHub pull request %d is mergeable", pull.Num) From 3d4fa63c59e59ef2642707230477744b2399b604 Mon Sep 17 00:00:00 2001 From: Ben Apprederisse Date: Thu, 24 Oct 2024 11:07:54 -0700 Subject: [PATCH 05/10] test: add unit tests --- server/events/vcs/github_client_test.go | 15 +++- .../ruleset-check-failed-other-atlantis.json | 80 +++++++++++++++++++ .../ruleset-check-pending-other-atlantis.json | 80 +++++++++++++++++++ 3 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-failed-other-atlantis.json create mode 100644 server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-pending-other-atlantis.json diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index ed8a57fbc3..69b1a0dedd 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -619,6 +619,7 @@ func TestGithubClient_PullIsMergeable(t *testing.T) { func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) { logger := logging.NewNoopLogger(t) vcsStatusName := "atlantis" + ignoreVCSStatusNames := []string{"other-atlantis"} cases := []struct { state string statusCheckRollupFilePath string @@ -709,6 +710,12 @@ func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) `"APPROVED"`, false, }, + { + "blocked", + "ruleset-check-pending-other-atlantis.json", + `"APPROVED"`, + true, + }, { "blocked", "ruleset-check-skipped.json", @@ -757,6 +764,12 @@ func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) `"APPROVED"`, false, }, + { + "blocked", + "ruleset-check-failed-other-atlantis.json", + `"APPROVED"`, + true, + }, { "blocked", "ruleset-check-passed.json", @@ -873,7 +886,7 @@ func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) }, }, models.PullRequest{ Num: 1, - }, vcsStatusName, []string{}) + }, vcsStatusName, ignoreVCSStatusNames) Ok(t, err) Equals(t, c.expMergeable, actMergeable) }) diff --git a/server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-failed-other-atlantis.json b/server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-failed-other-atlantis.json new file mode 100644 index 0000000000..38d291aa74 --- /dev/null +++ b/server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-failed-other-atlantis.json @@ -0,0 +1,80 @@ +{ + "data": { + "repository": { + "pullRequest": { + "reviewDecision": null, + "baseRef": { + "branchProtectionRule": { + "requiredStatusChecks": [] + }, + "rules": { + "pageInfo": { + "endCursor": "QWERTY", + "hasNextPage": false + }, + "nodes": [ + { + "type": "REQUIRED_STATUS_CHECKS", + "repositoryRuleset": { + "enforcement": "ACTIVE" + }, + "parameters": { + "requiredStatusChecks": [ + { + "context": "atlantis/apply" + }, + { + "context": "other-atlantis/apply" + } + ] + } + } + ] + } + }, + "commits": { + "nodes": [ + { + "commit": { + "statusCheckRollup": { + "contexts": { + "pageInfo": { + "endCursor": "QWERTY", + "hasNextPage": false + }, + "nodes": [ + { + "__typename": "StatusContext", + "context": "atlantis/apply", + "state": "PENDING", + "isRequired": true + }, + { + "__typename": "StatusContext", + "context": "atlantis/plan", + "state": "SUCCESS", + "isRequired": false + }, + { + "__typename": "StatusContext", + "context": "other-atlantis/apply", + "state": "FAILED", + "isRequired": true + }, + { + "__typename": "StatusContext", + "context": "other-atlantis/apply", + "state": "SUCCESS", + "isRequired": false + } + ] + } + } + } + } + ] + } + } + } + } +} diff --git a/server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-pending-other-atlantis.json b/server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-pending-other-atlantis.json new file mode 100644 index 0000000000..f0936309e0 --- /dev/null +++ b/server/events/vcs/testdata/github-pull-request-mergeability/ruleset-check-pending-other-atlantis.json @@ -0,0 +1,80 @@ +{ + "data": { + "repository": { + "pullRequest": { + "reviewDecision": null, + "baseRef": { + "branchProtectionRule": { + "requiredStatusChecks": [] + }, + "rules": { + "pageInfo": { + "endCursor": "QWERTY", + "hasNextPage": false + }, + "nodes": [ + { + "type": "REQUIRED_STATUS_CHECKS", + "repositoryRuleset": { + "enforcement": "ACTIVE" + }, + "parameters": { + "requiredStatusChecks": [ + { + "context": "atlantis/apply" + }, + { + "context": "other-atlantis/apply" + } + ] + } + } + ] + } + }, + "commits": { + "nodes": [ + { + "commit": { + "statusCheckRollup": { + "contexts": { + "pageInfo": { + "endCursor": "QWERTY", + "hasNextPage": false + }, + "nodes": [ + { + "__typename": "StatusContext", + "context": "atlantis/apply", + "state": "PENDING", + "isRequired": true + }, + { + "__typename": "StatusContext", + "context": "atlantis/plan", + "state": "SUCCESS", + "isRequired": false + }, + { + "__typename": "StatusContext", + "context": "other-atlantis/apply", + "state": "PENDING", + "isRequired": true + }, + { + "__typename": "StatusContext", + "context": "other-atlantis/plan", + "state": "PENDING", + "isRequired": false + } + ] + } + } + } + } + ] + } + } + } + } +} From e74cd145ab1729dea5f0366ec0da43dd288f0763 Mon Sep 17 00:00:00 2001 From: Ben Apprederisse Date: Thu, 24 Oct 2024 15:27:25 -0700 Subject: [PATCH 06/10] doc: server-configuration.md --- runatlantis.io/docs/server-configuration.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/runatlantis.io/docs/server-configuration.md b/runatlantis.io/docs/server-configuration.md index 610838f262..e881df3616 100644 --- a/runatlantis.io/docs/server-configuration.md +++ b/runatlantis.io/docs/server-configuration.md @@ -857,6 +857,19 @@ This is useful when you have many projects and want to keep the pull request cle Used for example with CDKTF pre-workflow hooks that dynamically generate Terraform files. +### `--ignore-vcs-status-names` + + ```bash + atlantis server --ignore-vcs-status-names="status1,status2" + # or + ATLANTIS_IGNORE_VCS_STATUS_NAMES=status1,status2 + ``` + + Comma separated list of VCS status names from other atlantis services. + When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks + (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) + from other atlantis services when checking if the PR is mergeable. + ### `--locking-db-type` ```bash From 5e062debf1e56e9877e2d110403c34655300df16 Mon Sep 17 00:00:00 2001 From: bakayolo Date: Sat, 2 Nov 2024 08:51:22 -0700 Subject: [PATCH 07/10] doc: PR comments --- cmd/server.go | 4 +++- runatlantis.io/docs/server-configuration.md | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/server.go b/cmd/server.go index 9a798417b2..074d345500 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -442,7 +442,9 @@ var stringFlags = map[string]stringFlag{ " If this argument is not provided, it defaults to Atlantis' data directory, determined by the --data-dir argument.", }, IgnoreVCSStatusNames: { - description: "Comma-separated list of other Atlantis servers to ignore for pull request statuses.", + description: "Comma separated list of VCS status names from other atlantis services." + + " When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) from other atlantis services when checking if the PR is mergeable." + + " Only for Github but can be extended to other VCS in the future.", defaultValue: DefaultIgnoreVCSStatusNames, }, VCSStatusName: { diff --git a/runatlantis.io/docs/server-configuration.md b/runatlantis.io/docs/server-configuration.md index e881df3616..2176181842 100644 --- a/runatlantis.io/docs/server-configuration.md +++ b/runatlantis.io/docs/server-configuration.md @@ -869,6 +869,7 @@ This is useful when you have many projects and want to keep the pull request cle When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) from other atlantis services when checking if the PR is mergeable. + Only for Github but can be extended to other VCS in the future. ### `--locking-db-type` From 568f3bbfeb852d3629e58c0be5fdf72d478d8c87 Mon Sep 17 00:00:00 2001 From: bakayolo Date: Sat, 2 Nov 2024 14:42:54 -0700 Subject: [PATCH 08/10] Update cmd/server.go Co-authored-by: Simon Heather <32168619+X-Guardian@users.noreply.github.com> Signed-off-by: bakayolo --- cmd/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index 074d345500..1edfd7f5f4 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -443,8 +443,8 @@ var stringFlags = map[string]stringFlag{ }, IgnoreVCSStatusNames: { description: "Comma separated list of VCS status names from other atlantis services." + - " When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) from other atlantis services when checking if the PR is mergeable." + - " Only for Github but can be extended to other VCS in the future.", + " When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) from other Atlantis services when checking if the PR is mergeable." + + " Currently only implemented for GitHub.", defaultValue: DefaultIgnoreVCSStatusNames, }, VCSStatusName: { From d94c9e7f01433a08a538349215ed75b225295890 Mon Sep 17 00:00:00 2001 From: bakayolo Date: Sat, 2 Nov 2024 14:43:02 -0700 Subject: [PATCH 09/10] Update runatlantis.io/docs/server-configuration.md Co-authored-by: Simon Heather <32168619+X-Guardian@users.noreply.github.com> Signed-off-by: bakayolo --- runatlantis.io/docs/server-configuration.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runatlantis.io/docs/server-configuration.md b/runatlantis.io/docs/server-configuration.md index 2176181842..3c062ae853 100644 --- a/runatlantis.io/docs/server-configuration.md +++ b/runatlantis.io/docs/server-configuration.md @@ -868,8 +868,8 @@ This is useful when you have many projects and want to keep the pull request cle Comma separated list of VCS status names from other atlantis services. When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) - from other atlantis services when checking if the PR is mergeable. - Only for Github but can be extended to other VCS in the future. + from other Atlantis services when checking if the PR is mergeable. + Currently only implemented for GitHub. ### `--locking-db-type` From 20d244226b721c1a737e991f8b23d1e2d3ae8a80 Mon Sep 17 00:00:00 2001 From: X-Guardian Date: Mon, 4 Nov 2024 21:40:51 +0000 Subject: [PATCH 10/10] Fix formatting Signed-off-by: X-Guardian --- cmd/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index 1edfd7f5f4..84ff1c23e9 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -442,8 +442,8 @@ var stringFlags = map[string]stringFlag{ " If this argument is not provided, it defaults to Atlantis' data directory, determined by the --data-dir argument.", }, IgnoreVCSStatusNames: { - description: "Comma separated list of VCS status names from other atlantis services." + - " When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) from other Atlantis services when checking if the PR is mergeable." + + description: "Comma separated list of VCS status names from other atlantis services." + + " When `gh-allow-mergeable-bypass-apply` is true, will ignore status checks (e.g. `status1/plan`, `status1/apply`, `status2/plan`, `status2/apply`) from other Atlantis services when checking if the PR is mergeable." + " Currently only implemented for GitHub.", defaultValue: DefaultIgnoreVCSStatusNames, },