From b8297bb1f819ef1e3e6258f7e20300a48d602b36 Mon Sep 17 00:00:00 2001 From: Stas Ostrovskyi Date: Thu, 8 Sep 2022 22:54:50 +0200 Subject: [PATCH] fix: Set mergeable correctly when branch protection doesn't require reviewers (#2470) * Allow no required reviewers * Fix comment in tests Co-authored-by: Stas Ostrovskyi --- ...hub-branch-protection-required-checks.json | 17 ++ .../fixtures/github-commit-check-suites.json | 169 ++++++++++++++++++ server/events/vcs/github_client.go | 2 +- server/events/vcs/github_client_test.go | 151 ++++++++++++++++ 4 files changed, 338 insertions(+), 1 deletion(-) create mode 100644 server/events/vcs/fixtures/github-branch-protection-required-checks.json create mode 100644 server/events/vcs/fixtures/github-commit-check-suites.json diff --git a/server/events/vcs/fixtures/github-branch-protection-required-checks.json b/server/events/vcs/fixtures/github-branch-protection-required-checks.json new file mode 100644 index 0000000000..9f422db9ea --- /dev/null +++ b/server/events/vcs/fixtures/github-branch-protection-required-checks.json @@ -0,0 +1,17 @@ +{ + "url": "https://api.github.com/repos/octocat/Hello-World/branches/master/protection", + "required_status_checks": { + "url": "https://api.github.com/repos/octocat/Hello-World/branches/master/protection/required_status_checks", + "strict": true, + "contexts": [ + "atlantis/apply" + ], + "contexts_url": "https://api.github.com/repos/octocat/Hello-World/branches/master/protection/required_status_checks/contexts", + "checks": [ + { + "context": "atlantis/apply", + "app_id": 123456 + } + ] + } +} diff --git a/server/events/vcs/fixtures/github-commit-check-suites.json b/server/events/vcs/fixtures/github-commit-check-suites.json new file mode 100644 index 0000000000..55f3ac972e --- /dev/null +++ b/server/events/vcs/fixtures/github-commit-check-suites.json @@ -0,0 +1,169 @@ +{ + "total_count": 1, + "check_suites": [ + { + "id": 1234567890, + "node_id": "CS_kwDOHE7PYM8AAAAB2iIZfQ", + "head_branch": "atlantis-patch-2", + "head_sha": "4273e07c528292222f119a040079093bf1f11232", + "status": "queued", + "conclusion": null, + "url": "https://api.github.com/repos/octocat/Hello-World/check-suites/1234567890", + "before": "0000000000000000000000000000000000000000", + "after": "4273e07c528292222f119a040079093bf1f11232", + "pull_requests": [ + { + "url": "https://api.github.com/repos/octocat/Hello-World/pulls/1", + "id": 1035065545, + "number": 1, + "head": { + "ref": "atlantis-patch-2", + "sha": "4273e07c528292222f119a040079093bf1f11232", + "repo": { + "id": 474926944, + "url": "https://api.github.com/repos/octocat/Hello-World", + "name": "Hello-World" + } + }, + "base": { + "ref": "master", + "sha": "6f5744874b33ceb6a5c91edc91085991dbd1f61a", + "repo": { + "id": 474926944, + "url": "https://api.github.com/repos/octocat/Hello-World", + "name": "Hello-World" + } + } + } + ], + "app": { + "id": 184783, + "slug": "atlantis", + "node_id": "A_kwHOBbMkBs4AAtHP", + "owner": { + "login": "octocat", + "id": 95626246, + "node_id": "O_kgDOBbMkBg", + "avatar_url": "https://avatars.githubusercontent.com/u/95626246?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": "Organization", + "site_admin": false + }, + "name": "atlantis", + "description": "", + "external_url": "https://atlantis.localhost/", + "html_url": "https://github.com/apps/atlantis", + "created_at": "2022-03-29T08:51:26Z", + "updated_at": "2022-07-01T11:35:37Z", + "permissions": { + "administration": "read", + "checks": "write", + "contents": "write", + "issues": "write", + "metadata": "read", + "pull_requests": "write", + "statuses": "write" + }, + "events": [] + }, + "created_at": "2022-08-24T07:06:21Z", + "updated_at": "2022-08-24T07:06:21Z", + "rerequestable": true, + "runs_rerequestable": true, + "latest_check_runs_count": 0, + "check_runs_url": "https://api.github.com/repos/octocat/Hello-World/check-suites/1234567890/check-runs", + "head_commit": { + "id": "4273e07c528292222f119a040079093bf1f11232", + "tree_id": "56781332464aabdfae51b7f37f72ffc6ce8ce54e", + "message": "test atlantis", + "timestamp": "2022-08-24T07:06:21Z", + "author": { + "name": "octocat", + "email": "octocat@noreply.github.com" + }, + "committer": { + "name": "GitHub", + "email": "noreply@github.com" + } + }, + "repository": { + "id": 474926944, + "node_id": "R_kgDOHE7PYA", + "name": "Hello-World", + "full_name": "octocat/Hello-World", + "private": true, + "owner": { + "login": "octocat", + "id": 95626246, + "node_id": "O_kgDOBbMkBg", + "avatar_url": "https://avatars.githubusercontent.com/u/95626246?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": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/octocat/Hello-World", + "description": null, + "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" + } + } + ] +} diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 321bd4ffe0..cc24ac922b 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -365,7 +365,7 @@ func (g *GithubClient) GetPullReviewDecision(repo models.Repo, pull models.PullR return approvalStatus, errors.Wrap(err, "getting reviewDecision") } - if query.Repository.PullRequest.ReviewDecision == "APPROVED" { + if query.Repository.PullRequest.ReviewDecision == "APPROVED" || len(query.Repository.PullRequest.ReviewDecision) == 0 { return true, nil } diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index 04afd084b3..ee447bf88b 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -586,6 +586,157 @@ func TestGithubClient_PullIsMergeable(t *testing.T) { } } +func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) { + vcsStatusName := "atlantis" + cases := []struct { + state string + reviewDecision string + expMergeable bool + }{ + { + "dirty", + `"REVIEW_REQUIRED"`, + false, + }, + { + "unknown", + `"REVIEW_REQUIRED"`, + false, + }, + { + "blocked", + `"REVIEW_REQUIRED"`, + false, + }, + { + "blocked", + `"APPROVED"`, + true, + }, + { + "blocked", + "null", + true, + }, + { + "behind", + `"REVIEW_REQUIRED"`, + false, + }, + { + "random", + `"REVIEW_REQUIRED"`, + false, + }, + { + "unstable", + `"REVIEW_REQUIRED"`, + true, + }, + { + "has_hooks", + `"APPROVED"`, + true, + }, + { + "clean", + `"APPROVED"`, + true, + }, + { + "", + `"APPROVED"`, + false, + }, + } + + // Use a real GitHub json response and edit the mergeable_state field. + jsBytes, err := os.ReadFile("fixtures/github-pull-request.json") + 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) + + // Branch protection Response + jsBytes, err = os.ReadFile("fixtures/github-branch-protection-required-checks.json") + Ok(t, err) + branchProtectionJSON := string(jsBytes) + + // List check suites Response + jsBytes, err = os.ReadFile("fixtures/github-commit-check-suites.json") + Ok(t, err) + checkSuites := string(jsBytes) + + for _, c := range cases { + t.Run(c.state, func(t *testing.T) { + response := strings.Replace(prJSON, + `"mergeable_state": "clean"`, + fmt.Sprintf(`"mergeable_state": "%s"`, c.state), + 1, + ) + + // reviewDecision Response + reviewDecision := fmt.Sprintf(`{ + "data": { + "repository": { + "pullRequest": { + "reviewDecision": %s + } + } + } + }`, c.reviewDecision) + + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + 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 + case "/api/graphql": + w.Write([]byte(reviewDecision)) // nolint: errcheck + case "/api/v3/repos/owner/repo/branches/master/protection": + w.Write([]byte(branchProtectionJSON)) // nolint: errcheck + case "/api/v3/repos/owner/repo/commits/new-topic/check-suites": + w.Write([]byte(checkSuites)) // nolint: errcheck + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{"user", "pass"}, vcs.GithubConfig{AllowMergeableBypassApply: true}, logging.NewNoopLogger(t)) + Ok(t, err) + defer disableSSLVerification()() + + actMergeable, err := client.PullIsMergeable(models.Repo{ + FullName: "owner/repo", + Owner: "owner", + Name: "repo", + CloneURL: "", + SanitizedCloneURL: "", + VCSHost: models.VCSHost{ + Type: models.Github, + Hostname: "github.com", + }, + }, models.PullRequest{ + Num: 1, + }, vcsStatusName) + Ok(t, err) + Equals(t, c.expMergeable, actMergeable) + }) + } +} + func TestGithubClient_MergePullHandlesError(t *testing.T) { cases := []struct { code int