Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "feat: filter out atlantis/apply from mergeability clause (#18… #1968

Merged
merged 2 commits into from
Jan 1, 2022
Merged

Revert "feat: filter out atlantis/apply from mergeability clause (#18… #1968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37f7af7
Revert "feat: filter out atlantis/apply from mergeability clause (#18…
Dec 31, 2021
9d2e4d2
Add missing import.
Dec 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/controllers/events/events_controller_e2e_test.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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