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

Make required atlantis/apply status check work with mergeable using --gh-allow-mergeable-bypass-apply #2436

Merged
merged 8 commits into from
Aug 18, 2022
Merged

Make required atlantis/apply status check work with mergeable using --gh-allow-mergeable-bypass-apply #2436

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7dec6c8
Make required apply work with mergeable
rayterrill Aug 9, 2022
86d912b
Fix lint hint
rayterrill Aug 9, 2022
436256a
Migrate mergeable approval check to graphql, update tests
rayterrill Aug 11, 2022
34c2382
Adding support for check suites
rayterrill Aug 12, 2022
a6f943a
Adding feature flag protection for new functionality
rayterrill Aug 12, 2022
4fcc565
Fix linting falsepos
rayterrill Aug 12, 2022
2f73b71
Adjusted logic to handle required check runs, added doc
rayterrill Aug 15, 2022
5d7c9cf
Merge branch 'master' into mergeable-apply-required
rayterrill Aug 16, 2022
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
148 changes: 148 additions & 0 deletions server/events/vcs/fixtures/github-commit-status-full.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
{
"state": "blocked",
"statuses": [
{
"url": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e",
"avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4",
"id": 16230299674,
"node_id": "SC_kwDOFRFvL88AAAADx2a4Gg",
"state": "success",
"description": "Plan succeeded.",
"target_url": "https://localhost/jobs/octocat/Hello-World/1/project1",
"context": "atlantis/plan: project1",
"created_at": "2022-02-10T15:26:01Z",
"updated_at": "2022-02-10T15:26:01Z"
},
{
"url": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e",
"avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4",
"id": 16230303174,
"node_id": "SC_kwDOFRFvL88AAAADx2bFxg",
"state": "success",
"description": "Plan succeeded.",
"target_url": "https://localhost/jobs/octocat/Hello-World/1/project2",
"context": "atlantis/plan: project2",
"created_at": "2022-02-10T15:26:12Z",
"updated_at": "2022-02-10T15:26:12Z"
},
{
"url": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e",
"avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4",
"id": 16230303679,
"node_id": "SC_kwDOFRFvL88AAAADx2bHvw",
"state": "success",
"description": "2/2 projects planned successfully.",
"target_url": "",
"context": "atlantis/plan",
"created_at": "2022-02-10T15:26:13Z",
"updated_at": "2022-02-10T15:26:13Z"
},
{
"url": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e",
"avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4",
"id": 16230307923,
"node_id": "SC_kwDOFRFvL88AAAADx2bYUw",
"state": "failure",
"description": "Apply failed.",
"target_url": "https://localhost/jobs/octocat/Hello-World/1/project1",
"context": "atlantis/apply: project1",
"created_at": "2022-02-10T15:26:27Z",
"updated_at": "2022-02-10T15:26:27Z"
},
{
"url": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e",
"avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4",
"id": 16230308153,
"node_id": "SC_kwDOFRFvL88AAAADx2bZOQ",
"state": "failure",
"description": "Apply failed.",
"target_url": "https://localhost/jobs/octocat/Hello-World/1/project2",
"context": "atlantis/apply: project2",
"created_at": "2022-02-10T15:26:27Z",
"updated_at": "2022-02-10T15:26:27Z"
},
{
"url": "https://api.github.com/repos/octocat/Hello-World/statuses/6dcb09b5b57875f334f61aebed695e2e4193db5e",
"avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4",
"id": 16230308528,
"node_id": "SC_kwDOFRFvL88AAAADx2basA",
"state": "failure",
"description": "0/2 projects applied successfully.",
"target_url": "",
"context": "atlantis/apply",
"created_at": "2022-02-10T15:26:28Z",
"updated_at": "2022-02-10T15:26:28Z"
}
],
"sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e",
"total_count": 0,
"repository": {
"id": 1296269,
"node_id": "MDEwOlJlcG9zaXRvcnkxMjk2MjY5",
"name": "Hello-World",
"full_name": "octocat/Hello-World",
"private": false,
"owner": {
"login": "octocat",
"id": 583231,
"node_id": "MDQ6VXNlcjU4MzIzMQ==",
"avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/octocat",
"html_url": "https://github.com/octocat",
"followers_url": "https://api.github.com/users/octocat/followers",
"following_url": "https://api.github.com/users/octocat/following{/other_user}",
"gists_url": "https://api.github.com/users/octocat/gists{/gist_id}",
"starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/octocat/subscriptions",
"organizations_url": "https://api.github.com/users/octocat/orgs",
"repos_url": "https://api.github.com/users/octocat/repos",
"events_url": "https://api.github.com/users/octocat/events{/privacy}",
"received_events_url": "https://api.github.com/users/octocat/received_events",
"type": "User",
"site_admin": false
},
"html_url": "https://github.com/octocat/Hello-World",
"description": "My first repository on GitHub!",
"fork": false,
"url": "https://api.github.com/repos/octocat/Hello-World",
"forks_url": "https://api.github.com/repos/octocat/Hello-World/forks",
"keys_url": "https://api.github.com/repos/octocat/Hello-World/keys{/key_id}",
"collaborators_url": "https://api.github.com/repos/octocat/Hello-World/collaborators{/collaborator}",
"teams_url": "https://api.github.com/repos/octocat/Hello-World/teams",
"hooks_url": "https://api.github.com/repos/octocat/Hello-World/hooks",
"issue_events_url": "https://api.github.com/repos/octocat/Hello-World/issues/events{/number}",
"events_url": "https://api.github.com/repos/octocat/Hello-World/events",
"assignees_url": "https://api.github.com/repos/octocat/Hello-World/assignees{/user}",
"branches_url": "https://api.github.com/repos/octocat/Hello-World/branches{/branch}",
"tags_url": "https://api.github.com/repos/octocat/Hello-World/tags",
"blobs_url": "https://api.github.com/repos/octocat/Hello-World/git/blobs{/sha}",
"git_tags_url": "https://api.github.com/repos/octocat/Hello-World/git/tags{/sha}",
"git_refs_url": "https://api.github.com/repos/octocat/Hello-World/git/refs{/sha}",
"trees_url": "https://api.github.com/repos/octocat/Hello-World/git/trees{/sha}",
"statuses_url": "https://api.github.com/repos/octocat/Hello-World/statuses/{sha}",
"languages_url": "https://api.github.com/repos/octocat/Hello-World/languages",
"stargazers_url": "https://api.github.com/repos/octocat/Hello-World/stargazers",
"contributors_url": "https://api.github.com/repos/octocat/Hello-World/contributors",
"subscribers_url": "https://api.github.com/repos/octocat/Hello-World/subscribers",
"subscription_url": "https://api.github.com/repos/octocat/Hello-World/subscription",
"commits_url": "https://api.github.com/repos/octocat/Hello-World/commits{/sha}",
"git_commits_url": "https://api.github.com/repos/octocat/Hello-World/git/commits{/sha}",
"comments_url": "https://api.github.com/repos/octocat/Hello-World/comments{/number}",
"issue_comment_url": "https://api.github.com/repos/octocat/Hello-World/issues/comments{/number}",
"contents_url": "https://api.github.com/repos/octocat/Hello-World/contents/{+path}",
"compare_url": "https://api.github.com/repos/octocat/Hello-World/compare/{base}...{head}",
"merges_url": "https://api.github.com/repos/octocat/Hello-World/merges",
"archive_url": "https://api.github.com/repos/octocat/Hello-World/{archive_format}{/ref}",
"downloads_url": "https://api.github.com/repos/octocat/Hello-World/downloads",
"issues_url": "https://api.github.com/repos/octocat/Hello-World/issues{/number}",
"pulls_url": "https://api.github.com/repos/octocat/Hello-World/pulls{/number}",
"milestones_url": "https://api.github.com/repos/octocat/Hello-World/milestones{/number}",
"notifications_url": "https://api.github.com/repos/octocat/Hello-World/notifications{?since,all,participating}",
"labels_url": "https://api.github.com/repos/octocat/Hello-World/labels{/name}",
"releases_url": "https://api.github.com/repos/octocat/Hello-World/releases{/id}",
"deployments_url": "https://api.github.com/repos/octocat/Hello-World/deployments"
},
"commit_url": "https://api.github.com/repos/octocat/Hello-World/commits/6dcb09b5b57875f334f61aebed695e2e4193db5e",
"url": "https://api.github.com/repos/octocat/Hello-World/commits/6dcb09b5b57875f334f61aebed695e2e4193db5e/status"
}
38 changes: 38 additions & 0 deletions server/events/vcs/github_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/base64"
"fmt"
"net/http"
"regexp"
"strings"
"time"

Expand Down Expand Up @@ -296,12 +297,45 @@ func (g *GithubClient) PullIsApproved(repo models.Repo, pull models.PullRequest)
return approvalStatus, nil
}

// Checks Statuses for PR, excluding atlantis apply. Returns true if all other statuses are not in failure.
func (g *GithubClient) GetStatus(repo models.Repo, pull *github.PullRequest) (bool, error) {
lilincmu marked this conversation as resolved.
Show resolved Hide resolved
status, _, err := g.client.Repositories.GetCombinedStatus(g.ctx, repo.Owner, repo.Name, *pull.Head.Ref, nil)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be #2601 - here the repo.Owner and repo.Name come from the PR Destination repository, and the pull.Head.Ref may be the branch name in the Fork (Source repository)

From https://docs.github.com/en/rest/commits/statuses#get-the-combined-status-for-a-specific-reference it's not clear how to do this for commits on a fork.

if err != nil {
return false, errors.Wrap(err, "getting combined status")
}

//compile for performance reasons
matcher, _ := regexp.Compile("atlantis/apply")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to pass vcsstatusname down here since it's actually configurable. See the counterpart of GitLab as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot about that - will fix.


for _, r := range status.Statuses {
if !matcher.MatchString(*r.Context) {
if *r.State == "failure" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to return false when checks are pending?

return false, nil
}
}
}

return true, nil
}

// PullIsMergeable returns true if the pull request is mergeable.
func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error) {
githubPR, err := g.GetPullRequest(repo, pull.Num)
if err != nil {
return false, errors.Wrap(err, "getting pull request")
}

approved, err := g.PullIsApproved(repo, pull)
lilincmu marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return false, errors.Wrap(err, "getting pull request approval")
}
fmt.Printf("Approval: %v\n", approved.IsApproved)
rayterrill marked this conversation as resolved.
Show resolved Hide resolved

status, err := g.GetStatus(repo, githubPR)
if err != nil {
return false, errors.Wrap(err, "getting pull request status")
}

state := githubPR.GetMergeableState()
// We map our mergeable check to when the GitHub merge button is clickable.
// This corresponds to the following states:
Expand All @@ -313,6 +347,10 @@ 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" && status && approved.IsApproved {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here is that state will always be blocked (assuming the pending apply status) but you can't know for certain why it is blocked (GitHub api does not specify the reason, only that it is blocked)

I'm not sure if other status and approval are the only things that would block a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndreZiviani for sure - I anticipated this discussion - that's definitely the majority of the issue here - trying to disentangle what's making up that state.

return true, nil
}

return false, nil
}
return true, nil
Expand Down
10 changes: 10 additions & 0 deletions server/events/vcs/github_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,11 @@ func TestGithubClient_PullIsMergeable(t *testing.T) {
Ok(t, err)
prJSON := string(jsBytes)

// Status Check Response
jsBytes, err = os.ReadFile("fixtures/github-commit-status-full.json")
Ok(t, err)
commitJSON := string(jsBytes)

for _, c := range cases {
t.Run(c.state, func(t *testing.T) {
response := strings.Replace(prJSON,
Expand All @@ -537,6 +542,11 @@ func TestGithubClient_PullIsMergeable(t *testing.T) {
case "/api/v3/repos/owner/repo/pulls/1":
w.Write([]byte(response)) // nolint: errcheck
return
case "/api/v3/repos/owner/repo/pulls/1/reviews?per_page=300":
w.Write([]byte("[]")) // nolint: errcheck
return
case "/api/v3/repos/owner/repo/commits/new-topic/status":
w.Write([]byte(commitJSON)) // nolint: errcheck
default:
t.Errorf("got unexpected request at %q", r.RequestURI)
http.Error(w, "not found", http.StatusNotFound)
Expand Down