feat: filter out atlantis/apply from mergeability clause (runatlantis…

…#1856)

Filter out atlantis apply status during mergeability check.

server/controllers/events/events_controller_e2e_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/runatlantis/atlantis/server/events/mocks"
 	"github.com/runatlantis/atlantis/server/events/mocks/matchers"
 	"github.com/runatlantis/atlantis/server/events/models"
+	"github.com/runatlantis/atlantis/server/events/vcs"
 	vcsmocks "github.com/runatlantis/atlantis/server/events/vcs/mocks"
 	"github.com/runatlantis/atlantis/server/events/webhooks"
 	"github.com/runatlantis/atlantis/server/events/yaml"
@@ -811,7 +812,7 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl
 
 	// Mocks.
 	e2eVCSClient := vcsmocks.NewMockClient()
-	e2eStatusUpdater := &events.DefaultCommitStatusUpdater{Client: e2eVCSClient}
+	e2eStatusUpdater := &events.DefaultCommitStatusUpdater{Client: e2eVCSClient, TitleBuilder: vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}}
 	e2eGithubGetter := mocks.NewMockGithubPullGetter()
 	e2eGitlabGetter := mocks.NewMockGitlabMergeRequestGetter()
 

server/controllers/github_app_controller.go

@@ -18,6 +18,7 @@ type GithubAppController struct {
 	GithubSetupComplete bool
 	GithubHostname      string
 	GithubOrg           string
+	GithubStatusName    string
 }
 
 type githubWebhook struct {
@@ -55,7 +56,7 @@ func (g *GithubAppController) ExchangeCode(w http.ResponseWriter, r *http.Reques
 
 	g.Logger.Debug("Exchanging GitHub app code for app credentials")
 	creds := &vcs.GithubAnonymousCredentials{}
-	client, err := vcs.NewGithubClient(g.GithubHostname, creds, g.Logger)
+	client, err := vcs.NewGithubClient(g.GithubHostname, creds, g.Logger, g.GithubStatusName)
 	if err != nil {
 		g.respond(w, logging.Error, http.StatusInternalServerError, "Failed to exchange code for github app: %s", err)
 		return

server/events/commit_status_updater.go

@@ -28,24 +28,23 @@ import (
 type CommitStatusUpdater interface {
 	// UpdateCombined updates the combined status of the head commit of pull.
 	// A combined status represents all the projects modified in the pull.
-	UpdateCombined(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command models.CommandName) error
+	UpdateCombined(repo models.Repo, pull models.PullRequest, status models.CommitStatus, cmdName models.CommandName) error
 	// UpdateCombinedCount updates the combined status to reflect the
 	// numSuccess out of numTotal.
-	UpdateCombinedCount(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command models.CommandName, numSuccess int, numTotal int) error
+	UpdateCombinedCount(repo models.Repo, pull models.PullRequest, status models.CommitStatus, cmdName models.CommandName, numSuccess int, numTotal int) error
 	// UpdateProject sets the commit status for the project represented by
 	// ctx.
 	UpdateProject(ctx models.ProjectCommandContext, cmdName models.CommandName, status models.CommitStatus, url string) error
 }
 
 // DefaultCommitStatusUpdater implements CommitStatusUpdater.
 type DefaultCommitStatusUpdater struct {
-	Client vcs.Client
-	// StatusName is the name used to identify Atlantis when creating PR statuses.
-	StatusName string
+	Client       vcs.Client
+	TitleBuilder vcs.StatusTitleBuilder
 }
 
-func (d *DefaultCommitStatusUpdater) UpdateCombined(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command models.CommandName) error {
-	src := fmt.Sprintf("%s/%s", d.StatusName, command.String())
+func (d *DefaultCommitStatusUpdater) UpdateCombined(repo models.Repo, pull models.PullRequest, status models.CommitStatus, cmdName models.CommandName) error {
+	src := d.TitleBuilder.Build(cmdName.String())
 	var descripWords string
 	switch status {
 	case models.PendingCommitStatus:
@@ -55,15 +54,15 @@ func (d *DefaultCommitStatusUpdater) UpdateCombined(repo models.Repo, pull model
 	case models.SuccessCommitStatus:
 		descripWords = "succeeded."
 	}
-	descrip := fmt.Sprintf("%s %s", strings.Title(command.String()), descripWords)
+	descrip := fmt.Sprintf("%s %s", strings.Title(cmdName.String()), descripWords)
 	return d.Client.UpdateStatus(repo, pull, status, src, descrip, "")
 }
 
-func (d *DefaultCommitStatusUpdater) UpdateCombinedCount(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command models.CommandName, numSuccess int, numTotal int) error {
-	src := fmt.Sprintf("%s/%s", d.StatusName, command.String())
+func (d *DefaultCommitStatusUpdater) UpdateCombinedCount(repo models.Repo, pull models.PullRequest, status models.CommitStatus, cmdName models.CommandName, numSuccess int, numTotal int) error {
+	src := d.TitleBuilder.Build(cmdName.String())
 	cmdVerb := "unknown"
 
-	switch command {
+	switch cmdName {
 	case models.PlanCommand:
 		cmdVerb = "planned"
 	case models.PolicyCheckCommand:
@@ -80,7 +79,10 @@ func (d *DefaultCommitStatusUpdater) UpdateProject(ctx models.ProjectCommandCont
 	if projectID == "" {
 		projectID = fmt.Sprintf("%s/%s", ctx.RepoRelDir, ctx.Workspace)
 	}
-	src := fmt.Sprintf("%s/%s: %s", d.StatusName, cmdName.String(), projectID)
+
+	src := d.TitleBuilder.Build(cmdName.String(), vcs.StatusTitleOptions{
+		ProjectName: projectID,
+	})
 	var descripWords string
 	switch status {
 	case models.PendingCommitStatus:

server/events/commit_status_updater_test.go

@@ -20,6 +20,7 @@ import (
 	. "github.com/petergtz/pegomock"
 	"github.com/runatlantis/atlantis/server/events"
 	"github.com/runatlantis/atlantis/server/events/models"
+	"github.com/runatlantis/atlantis/server/events/vcs"
 	"github.com/runatlantis/atlantis/server/events/vcs/mocks"
 	. "github.com/runatlantis/atlantis/testing"
 )
@@ -66,7 +67,9 @@ func TestUpdateCombined(t *testing.T) {
 		t.Run(c.expDescrip, func(t *testing.T) {
 			RegisterMockTestingT(t)
 			client := mocks.NewMockClient()
-			s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis"}
+
+			titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}
+			s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
 			err := s.UpdateCombined(models.Repo{}, models.PullRequest{}, c.status, c.command)
 			Ok(t, err)
 
@@ -132,11 +135,12 @@ func TestUpdateCombinedCount(t *testing.T) {
 		t.Run(c.expDescrip, func(t *testing.T) {
 			RegisterMockTestingT(t)
 			client := mocks.NewMockClient()
-			s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis-test"}
+			titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis-test"}
+			s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
 			err := s.UpdateCombinedCount(models.Repo{}, models.PullRequest{}, c.status, c.command, c.numSuccess, c.numTotal)
 			Ok(t, err)
 
-			expSrc := fmt.Sprintf("%s/%s", s.StatusName, c.command)
+			expSrc := fmt.Sprintf("%s/%s", titleBuilder.TitlePrefix, c.command)
 			client.VerifyWasCalledOnce().UpdateStatus(models.Repo{}, models.PullRequest{}, c.status, expSrc, c.expDescrip, "")
 		})
 	}
@@ -169,7 +173,8 @@ func TestDefaultCommitStatusUpdater_UpdateProjectSrc(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.expSrc, func(t *testing.T) {
 			client := mocks.NewMockClient()
-			s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis"}
+			titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}
+			s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
 			err := s.UpdateProject(models.ProjectCommandContext{
 				ProjectName: c.projectName,
 				RepoRelDir:  c.repoRelDir,
@@ -227,7 +232,8 @@ func TestDefaultCommitStatusUpdater_UpdateProject(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.expDescrip, func(t *testing.T) {
 			client := mocks.NewMockClient()
-			s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis"}
+			titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}
+			s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
 			err := s.UpdateProject(models.ProjectCommandContext{
 				RepoRelDir: ".",
 				Workspace:  "default",
@@ -245,7 +251,8 @@ func TestDefaultCommitStatusUpdater_UpdateProject(t *testing.T) {
 func TestDefaultCommitStatusUpdater_UpdateProjectCustomStatusName(t *testing.T) {
 	RegisterMockTestingT(t)
 	client := mocks.NewMockClient()
-	s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "custom"}
+	titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "custom"}
+	s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
 	err := s.UpdateProject(models.ProjectCommandContext{
 		RepoRelDir: ".",
 		Workspace:  "default",

server/events/vcs/github_client.go

@@ -33,15 +33,18 @@ import (
 
 // maxCommentLength is the maximum number of chars allowed in a single comment
 // by GitHub.
-const maxCommentLength = 65536
+const (
+	maxCommentLength = 65536
+)
 
 // GithubClient is used to perform GitHub actions.
 type GithubClient struct {
-	user           string
-	client         *github.Client
-	v4MutateClient *graphql.Client
-	ctx            context.Context
-	logger         logging.SimpleLogging
+	user               string
+	client             *github.Client
+	v4MutateClient     *graphql.Client
+	ctx                context.Context
+	logger             logging.SimpleLogging
+	statusTitleMatcher StatusTitleMatcher
 }
 
 // GithubAppTemporarySecrets holds app credentials obtained from github after creation.
@@ -59,7 +62,7 @@ type GithubAppTemporarySecrets struct {
 }
 
 // NewGithubClient returns a valid GitHub client.
-func NewGithubClient(hostname string, credentials GithubCredentials, logger logging.SimpleLogging) (*GithubClient, error) {
+func NewGithubClient(hostname string, credentials GithubCredentials, logger logging.SimpleLogging, commitStatusPrefix string) (*GithubClient, error) {
 	transport, err := credentials.Client()
 	if err != nil {
 		return nil, errors.Wrap(err, "error initializing github authentication transport")
@@ -99,11 +102,12 @@ func NewGithubClient(hostname string, credentials GithubCredentials, logger logg
 		return nil, errors.Wrap(err, "getting user")
 	}
 	return &GithubClient{
-		user:           user,
-		client:         client,
-		v4MutateClient: v4MutateClient,
-		ctx:            context.Background(),
-		logger:         logger,
+		user:               user,
+		client:             client,
+		v4MutateClient:     v4MutateClient,
+		ctx:                context.Background(),
+		logger:             logger,
+		statusTitleMatcher: StatusTitleMatcher{TitlePrefix: commitStatusPrefix},
 	}, nil
 }
 
@@ -280,8 +284,40 @@ func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
 	//            hooks. Merging is allowed (green box).
 	// See: https://github.com/octokit/octokit.net/issues/1763
 	if state != "clean" && state != "unstable" && state != "has_hooks" {
+
+		if state != "blocked" {
+			return false, nil
+		}
+
+		return g.getSupplementalMergeability(repo, pull)
+	}
+	return true, nil
+}
+
+// Checks to make sure that all statuses are passing except the atlantis/apply. If we only rely on GetMergeableState,
+// we can run into issues where if an apply failed, we can never apply again due to mergeability failures.
+func (g *GithubClient) getSupplementalMergeability(repo models.Repo, pull models.PullRequest) (bool, error) {
+	statuses, err := g.getRepoStatuses(repo, pull)
+
+	if err != nil {
+		return false, errors.Wrapf(err, "fetching repo statuses for repo: %s, and pull number: %d", repo.FullName, pull.Num)
+	}
+
+	for _, status := range statuses {
+		state := status.GetState()
+
+		if g.statusTitleMatcher.MatchesCommand(status.GetContext(), "apply") ||
+			state == "success" {
+			continue
+
+		}
+
+		// we either have a failure or a pending status check
+		// hence the PR is not mergeable
 		return false, nil
 	}
+
+	// all our status checks are successful by our definition,
 	return true, nil
 }
 
@@ -312,6 +348,38 @@ func (g *GithubClient) GetPullRequest(repo models.Repo, num int) (*github.PullRe
 	return pull, err
 }
 
+func (g *GithubClient) getRepoStatuses(repo models.Repo, pull models.PullRequest) ([]*github.RepoStatus, error) {
+	// Get Combined statuses
+
+	nextPage := 0
+
+	var result []*github.RepoStatus
+
+	for {
+		opts := github.ListOptions{
+			// explicit default
+			// https://developer.github.com/v3/repos/statuses/#list-commit-statuses-for-a-reference
+			PerPage: 100,
+		}
+		if nextPage != 0 {
+			opts.Page = nextPage
+		}
+
+		combinedStatus, response, err := g.client.Repositories.GetCombinedStatus(g.ctx, repo.Owner, repo.Name, pull.HeadCommit, &opts)
+		result = append(result, combinedStatus.Statuses...)
+
+		if err != nil {
+			return nil, err
+		}
+		if response.NextPage == 0 {
+			break
+		}
+		nextPage = response.NextPage
+	}
+
+	return result, nil
+}
+
 // UpdateStatus updates the status badge on the pull request.
 // See https://github.com/blog/1227-commit-status-api.
 func (g *GithubClient) UpdateStatus(repo models.Repo, pull models.PullRequest, state models.CommitStatus, src string, description string, url string) error {

server/events/vcs/github_client_internal_test.go

@@ -22,14 +22,14 @@ import (
 
 // If the hostname is github.com, should use normal BaseURL.
 func TestNewGithubClient_GithubCom(t *testing.T) {
-	client, err := NewGithubClient("github.com", &GithubUserCredentials{"user", "pass"}, logging.NewNoopLogger(t))
+	client, err := NewGithubClient("github.com", &GithubUserCredentials{"user", "pass"}, logging.NewNoopLogger(t), "atlantis")
 	Ok(t, err)
 	Equals(t, "https://api.github.com/", client.client.BaseURL.String())
 }
 
 // If the hostname is a non-github hostname should use the right BaseURL.
 func TestNewGithubClient_NonGithub(t *testing.T) {
-	client, err := NewGithubClient("example.com", &GithubUserCredentials{"user", "pass"}, logging.NewNoopLogger(t))
+	client, err := NewGithubClient("example.com", &GithubUserCredentials{"user", "pass"}, logging.NewNoopLogger(t), "atlantis")
 	Ok(t, err)
 	Equals(t, "https://example.com/api/v3/", client.client.BaseURL.String())
 	// If possible in the future, test the GraphQL client's URL as well. But at the