From 6436180f56ef5e02acca04b0fca008cbe279796f Mon Sep 17 00:00:00 2001 From: Tymofii Polekhin Date: Wed, 29 Mar 2023 13:28:52 +0200 Subject: [PATCH 1/2] fix: github branch protection not supported --- server/events/vcs/github_client.go | 11 ++++- server/events/vcs/github_client_test.go | 59 ++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 10b31add1e..0dd2989795 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -34,6 +34,10 @@ import ( // by GitHub. const maxCommentLength = 65536 +// Error message GitHub API returns if branch protection is not available +// in current repository +const githubBranchProtectionNotAvailable string = "Upgrade to GitHub Pro or make this repository public to enable this feature." + var ( clientMutationID = githubv4.NewString("atlantis") pullRequestDismissalMessage = *githubv4.NewString("Dismissing reviews because of plan changes") @@ -570,6 +574,11 @@ func (g *GithubClient) UpdateStatus(repo models.Repo, pull models.PullRequest, s return err } +func isBranchProtectionNotAvailable(err error) bool { + errorResponse, ok := err.(*github.ErrorResponse) + return ok && errorResponse.Message == githubBranchProtectionNotAvailable +} + // MergePull merges the pull request. func (g *GithubClient) MergePull(pull models.PullRequest, pullOptions models.PullRequestOptions) error { // Users can set their repo to disallow certain types of merging. @@ -581,7 +590,7 @@ func (g *GithubClient) MergePull(pull models.PullRequest, pullOptions models.Pul } protection, _, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner.GetLogin(), *repo.Name, pull.BaseBranch) if err != nil { - if !errors.Is(err, github.ErrBranchNotProtected) { + if !errors.Is(err, github.ErrBranchNotProtected) && !isBranchProtectionNotAvailable(err) { return errors.Wrap(err, "getting branch protection rules") } } diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index 3da4ad0b7c..616b656914 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -841,6 +841,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash bool requireLinearHistory bool protectedBranch bool + protectionAvailabale bool expMethod string }{ "all true": { @@ -849,6 +850,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, + protectionAvailabale: true, expMethod: "merge", }, "all false (edge case)": { @@ -857,6 +859,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: false, + protectionAvailabale: true, expMethod: "merge", }, "merge: false rebase: true squash: true": { @@ -865,6 +868,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, + protectionAvailabale: true, expMethod: "rebase", }, "merge: false rebase: false squash: true": { @@ -873,6 +877,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, + protectionAvailabale: true, expMethod: "squash", }, "merge: false rebase: true squash: false": { @@ -881,6 +886,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: false, + protectionAvailabale: true, expMethod: "rebase", }, "protected, all true, rlh: false": { @@ -889,6 +895,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: true, + protectionAvailabale: true, expMethod: "merge", }, "protected, all false (edge case), rlh: false": { @@ -897,6 +904,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: true, + protectionAvailabale: true, expMethod: "merge", }, "protected, merge: false rebase: true squash: true, rlh: false": { @@ -905,6 +913,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: true, + protectionAvailabale: true, expMethod: "rebase", }, "protected, merge: false rebase: false squash: true, rlh: false": { @@ -913,6 +922,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: true, + protectionAvailabale: true, expMethod: "squash", }, "protected, merge: false rebase: true squash: false, rlh: false": { @@ -921,6 +931,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: true, + protectionAvailabale: true, expMethod: "rebase", }, "protected, all true, rlh: true": { @@ -929,6 +940,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: true, protectedBranch: true, + protectionAvailabale: true, expMethod: "rebase", }, "protected, all false (edge case), rlh: true": { @@ -937,6 +949,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: true, protectedBranch: true, + protectionAvailabale: true, expMethod: "merge", }, "protected, merge: false rebase: true squash: true, rlh: true": { @@ -945,6 +958,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: true, protectedBranch: true, + protectionAvailabale: true, expMethod: "rebase", }, "protected, merge: false rebase: false squash: true, rlh: true": { @@ -953,6 +967,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: true, protectedBranch: true, + protectionAvailabale: true, expMethod: "squash", }, "protected, merge: false rebase: true squash: false, rlh: true": { @@ -961,6 +976,43 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: true, protectedBranch: true, + protectionAvailabale: true, + expMethod: "rebase", + }, + "protection not supported, all true": { + allowMerge: true, + allowRebase: true, + allowSquash: true, + requireLinearHistory: false, + protectedBranch: false, + protectionAvailabale: false, + expMethod: "merge", + }, + "protection not supported, merge: false, rebase: true, squash: true": { + allowMerge: false, + allowRebase: true, + allowSquash: true, + requireLinearHistory: false, + protectedBranch: false, + protectionAvailabale: false, + expMethod: "rebase", + }, + "protection not supported, merge: false, rebase: false, squash: true": { + allowMerge: false, + allowRebase: false, + allowSquash: true, + requireLinearHistory: false, + protectedBranch: false, + protectionAvailabale: false, + expMethod: "squash", + }, + "protection not supported, merge: false, rebase: true, squash: false": { + allowMerge: false, + allowRebase: true, + allowSquash: false, + requireLinearHistory: false, + protectedBranch: false, + protectionAvailabale: false, expMethod: "rebase", }, } @@ -999,11 +1051,14 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { w.Write([]byte(resp)) // nolint: errcheck return case "/api/v3/repos/runatlantis/atlantis/branches/master/protection": - if c.protectedBranch { + if c.protectedBranch && c.protectionAvailabale { w.Write([]byte(protected)) // nolint: errcheck - } else { + } else if !c.protectedBranch && c.protectionAvailabale { w.WriteHeader(404) w.Write([]byte("{\"message\":\"Branch not protected\"}")) // nolint: errcheck + } else if !c.protectionAvailabale { + w.WriteHeader(403) + w.Write([]byte("{\"message\":\"Upgrade to GitHub Pro or make this repository public to enable this feature.\"}")) // nolint: errcheck } return case "/api/v3/repos/runatlantis/atlantis/pulls/1/merge": From 14d822c06a1b86391654431c2637896617dbabb4 Mon Sep 17 00:00:00 2001 From: Tymofii Polekhin Date: Wed, 29 Mar 2023 16:23:22 +0200 Subject: [PATCH 2/2] fix: typo protectionAvailable --- server/events/vcs/github_client_test.go | 46 ++++++++++++------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index 616b656914..ddc658aca8 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -841,7 +841,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash bool requireLinearHistory bool protectedBranch bool - protectionAvailabale bool + protectionAvailable bool expMethod string }{ "all true": { @@ -850,7 +850,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "merge", }, "all false (edge case)": { @@ -859,7 +859,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "merge", }, "merge: false rebase: true squash: true": { @@ -868,7 +868,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "rebase", }, "merge: false rebase: false squash: true": { @@ -877,7 +877,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "squash", }, "merge: false rebase: true squash: false": { @@ -886,7 +886,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "rebase", }, "protected, all true, rlh: false": { @@ -895,7 +895,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "merge", }, "protected, all false (edge case), rlh: false": { @@ -904,7 +904,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "merge", }, "protected, merge: false rebase: true squash: true, rlh: false": { @@ -913,7 +913,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "rebase", }, "protected, merge: false rebase: false squash: true, rlh: false": { @@ -922,7 +922,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "squash", }, "protected, merge: false rebase: true squash: false, rlh: false": { @@ -931,7 +931,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "rebase", }, "protected, all true, rlh: true": { @@ -940,7 +940,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: true, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "rebase", }, "protected, all false (edge case), rlh: true": { @@ -949,7 +949,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: true, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "merge", }, "protected, merge: false rebase: true squash: true, rlh: true": { @@ -958,7 +958,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: true, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "rebase", }, "protected, merge: false rebase: false squash: true, rlh: true": { @@ -967,7 +967,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: true, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "squash", }, "protected, merge: false rebase: true squash: false, rlh: true": { @@ -976,7 +976,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: true, protectedBranch: true, - protectionAvailabale: true, + protectionAvailable: true, expMethod: "rebase", }, "protection not supported, all true": { @@ -985,7 +985,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: false, + protectionAvailable: false, expMethod: "merge", }, "protection not supported, merge: false, rebase: true, squash: true": { @@ -994,7 +994,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: false, + protectionAvailable: false, expMethod: "rebase", }, "protection not supported, merge: false, rebase: false, squash: true": { @@ -1003,7 +1003,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: true, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: false, + protectionAvailable: false, expMethod: "squash", }, "protection not supported, merge: false, rebase: true, squash: false": { @@ -1012,7 +1012,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { allowSquash: false, requireLinearHistory: false, protectedBranch: false, - protectionAvailabale: false, + protectionAvailable: false, expMethod: "rebase", }, } @@ -1051,12 +1051,12 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { w.Write([]byte(resp)) // nolint: errcheck return case "/api/v3/repos/runatlantis/atlantis/branches/master/protection": - if c.protectedBranch && c.protectionAvailabale { + if c.protectedBranch && c.protectionAvailable { w.Write([]byte(protected)) // nolint: errcheck - } else if !c.protectedBranch && c.protectionAvailabale { + } else if !c.protectedBranch && c.protectionAvailable { w.WriteHeader(404) w.Write([]byte("{\"message\":\"Branch not protected\"}")) // nolint: errcheck - } else if !c.protectionAvailabale { + } else if !c.protectionAvailable { w.WriteHeader(403) w.Write([]byte("{\"message\":\"Upgrade to GitHub Pro or make this repository public to enable this feature.\"}")) // nolint: errcheck }