Skip to content

Commit

Permalink
Revert "feat: filter out atlantis/apply from mergeability clause (#18… (
Browse files Browse the repository at this point in the history 
runatlantis#1968)

* Revert "feat: filter out atlantis/apply from mergeability clause (runatlantis#1856)"

This reverts commit d01796b.

* Add missing import.
  • Loading branch information
@nishkrishnan @krrrr38
nishkrishnan authored and krrrr38 committed Dec 16, 2022
1 parent c10aa16 commit 5641ff0
Show file tree
Hide file tree
Showing 11 changed files with 66 additions and 315 deletions.
2 changes: 1 addition & 1 deletion server/controllers/events/events_controller_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl

// Mocks.
e2eVCSClient := vcsmocks.NewMockClient()
e2eStatusUpdater := &events.DefaultCommitStatusUpdater{Client: e2eVCSClient, TitleBuilder: vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}}
e2eStatusUpdater := &events.DefaultCommitStatusUpdater{Client: e2eVCSClient}
e2eGithubGetter := mocks.NewMockGithubPullGetter()
e2eGitlabGetter := mocks.NewMockGitlabMergeRequestGetter()
projectCmdOutputHandler := handlermocks.NewMockProjectCommandOutputHandler()
Expand Down
3 changes: 1 addition & 2 deletions server/controllers/github_app_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ type GithubAppController struct {
GithubSetupComplete bool
GithubHostname string
GithubOrg string
GithubStatusName string
}

type githubWebhook struct {
Expand Down Expand Up @@ -56,7 +55,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, g.GithubStatusName)
client, err := vcs.NewGithubClient(g.GithubHostname, creds, g.Logger)
if err != nil {
g.respond(w, logging.Error, http.StatusInternalServerError, "Failed to exchange code for github app: %s", err)
return
Expand Down
40 changes: 22 additions & 18 deletions server/events/commit_status_updater.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,32 +28,42 @@ 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, cmdName models.CommandName) error
UpdateCombined(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command 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, cmdName models.CommandName, numSuccess int, numTotal int) error
UpdateCombinedCount(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command 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
TitleBuilder vcs.StatusTitleBuilder
Client vcs.Client
// StatusName is the name used to identify Atlantis when creating PR statuses.
StatusName string
}

func (d *DefaultCommitStatusUpdater) UpdateCombined(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command models.CommandName) error {
src := d.TitleBuilder.Build(command.String())
descrip := fmt.Sprintf("%s %s", strings.Title(command.String()), d.statusDescription(status))
src := fmt.Sprintf("%s/%s", d.StatusName, command.String())
var descripWords string
switch status {
case models.PendingCommitStatus:
descripWords = "in progress..."
case models.FailedCommitStatus:
descripWords = "failed."
case models.SuccessCommitStatus:
descripWords = "succeeded."
}
descrip := fmt.Sprintf("%s %s", strings.Title(command.String()), descripWords)
return d.Client.UpdateStatus(repo, pull, status, src, descrip, "")
}

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())
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())
cmdVerb := "unknown"

switch cmdName {
switch command {
case models.PlanCommand:
cmdVerb = "planned"
case models.PolicyCheckCommand:
Expand All @@ -70,14 +80,7 @@ func (d *DefaultCommitStatusUpdater) UpdateProject(ctx models.ProjectCommandCont
if projectID == "" {
projectID = fmt.Sprintf("%s/%s", ctx.RepoRelDir, ctx.Workspace)
}
src := d.TitleBuilder.Build(cmdName.String(), vcs.StatusTitleOptions{
ProjectName: projectID,
})
descrip := fmt.Sprintf("%s %s", strings.Title(cmdName.String()), d.statusDescription(status))
return d.Client.UpdateStatus(ctx.BaseRepo, ctx.Pull, status, src, descrip, url)
}

func (d *DefaultCommitStatusUpdater) statusDescription(status models.CommitStatus) string {
src := fmt.Sprintf("%s/%s: %s", d.StatusName, cmdName.String(), projectID)
var descripWords string
switch status {
case models.PendingCommitStatus:
Expand All @@ -88,5 +91,6 @@ func (d *DefaultCommitStatusUpdater) statusDescription(status models.CommitStatu
descripWords = "succeeded."
}

return descripWords
descrip := fmt.Sprintf("%s %s", strings.Title(cmdName.String()), descripWords)
return d.Client.UpdateStatus(ctx.BaseRepo, ctx.Pull, status, src, descrip, url)
}
19 changes: 6 additions & 13 deletions server/events/commit_status_updater_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ 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"
)
Expand Down Expand Up @@ -67,9 +66,7 @@ func TestUpdateCombined(t *testing.T) {
t.Run(c.expDescrip, func(t *testing.T) {
RegisterMockTestingT(t)
client := mocks.NewMockClient()

titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}
s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis"}
err := s.UpdateCombined(models.Repo{}, models.PullRequest{}, c.status, c.command)
Ok(t, err)

Expand Down Expand Up @@ -135,12 +132,11 @@ func TestUpdateCombinedCount(t *testing.T) {
t.Run(c.expDescrip, func(t *testing.T) {
RegisterMockTestingT(t)
client := mocks.NewMockClient()
titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis-test"}
s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis-test"}
err := s.UpdateCombinedCount(models.Repo{}, models.PullRequest{}, c.status, c.command, c.numSuccess, c.numTotal)
Ok(t, err)

expSrc := fmt.Sprintf("%s/%s", titleBuilder.TitlePrefix, c.command)
expSrc := fmt.Sprintf("%s/%s", s.StatusName, c.command)
client.VerifyWasCalledOnce().UpdateStatus(models.Repo{}, models.PullRequest{}, c.status, expSrc, c.expDescrip, "")
})
}
Expand Down Expand Up @@ -173,8 +169,7 @@ func TestDefaultCommitStatusUpdater_UpdateProjectSrc(t *testing.T) {
for _, c := range cases {
t.Run(c.expSrc, func(t *testing.T) {
client := mocks.NewMockClient()
titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}
s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis"}
err := s.UpdateProject(models.ProjectCommandContext{
ProjectName: c.projectName,
RepoRelDir: c.repoRelDir,
Expand Down Expand Up @@ -232,8 +227,7 @@ func TestDefaultCommitStatusUpdater_UpdateProject(t *testing.T) {
for _, c := range cases {
t.Run(c.expDescrip, func(t *testing.T) {
client := mocks.NewMockClient()
titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "atlantis"}
s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "atlantis"}
err := s.UpdateProject(models.ProjectCommandContext{
RepoRelDir: ".",
Workspace: "default",
Expand All @@ -251,8 +245,7 @@ func TestDefaultCommitStatusUpdater_UpdateProject(t *testing.T) {
func TestDefaultCommitStatusUpdater_UpdateProjectCustomStatusName(t *testing.T) {
RegisterMockTestingT(t)
client := mocks.NewMockClient()
titleBuilder := vcs.StatusTitleBuilder{TitlePrefix: "custom"}
s := events.DefaultCommitStatusUpdater{Client: client, TitleBuilder: titleBuilder}
s := events.DefaultCommitStatusUpdater{Client: client, StatusName: "custom"}
err := s.UpdateProject(models.ProjectCommandContext{
RepoRelDir: ".",
Workspace: "default",
Expand Down
92 changes: 12 additions & 80 deletions server/events/vcs/github_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,15 @@ 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
statusTitleMatcher StatusTitleMatcher
user string
client *github.Client
v4MutateClient *graphql.Client
ctx context.Context
logger logging.SimpleLogging
}

// GithubAppTemporarySecrets holds app credentials obtained from github after creation.
Expand All @@ -62,7 +59,7 @@ type GithubAppTemporarySecrets struct {
}

// NewGithubClient returns a valid GitHub client.
func NewGithubClient(hostname string, credentials GithubCredentials, logger logging.SimpleLogging, commitStatusPrefix string) (*GithubClient, error) {
func NewGithubClient(hostname string, credentials GithubCredentials, logger logging.SimpleLogging) (*GithubClient, error) {
transport, err := credentials.Client()
if err != nil {
return nil, errors.Wrap(err, "error initializing github authentication transport")
Expand Down Expand Up @@ -102,12 +99,11 @@ 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,
statusTitleMatcher: StatusTitleMatcher{TitlePrefix: commitStatusPrefix},
user: user,
client: client,
v4MutateClient: v4MutateClient,
ctx: context.Background(),
logger: logger,
}, nil
}

Expand Down Expand Up @@ -284,40 +280,8 @@ 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
}

Expand Down Expand Up @@ -348,38 +312,6 @@ 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 {
Expand Down
4 changes: 2 additions & 2 deletions server/events/vcs/github_client_internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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), "atlantis")
client, err := NewGithubClient("github.com", &GithubUserCredentials{"user", "pass"}, logging.NewNoopLogger(t))
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), "atlantis")
client, err := NewGithubClient("example.com", &GithubUserCredentials{"user", "pass"}, logging.NewNoopLogger(t))
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
Expand Down
Loading

0 comments on commit 5641ff0

Please sign in to comment.