From 6dabc454147103d037931d343956a59833aec0d4 Mon Sep 17 00:00:00 2001 From: Dylan Page Date: Tue, 11 Apr 2023 16:21:14 -0400 Subject: [PATCH] Revert "fix(github): branch protection not supported (#3276)" This reverts commit ebc06c19c6cd5234048e40a2c5611c182a38e5c0. Revert "fix: allow `Require Linear History` when selecting merge method (#3211)" This reverts commit 7a3382822789c1d2851940b41c85c95931e9b0de. --- server/events/vcs/github_client.go | 21 +- server/events/vcs/github_client_test.go | 220 +++--------------- ...nch-protection-require-linear-history.json | 6 - 3 files changed, 27 insertions(+), 220 deletions(-) delete mode 100644 server/events/vcs/testdata/github-branch-protection-require-linear-history.json diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index b10b00ee6d..b9fac0c85f 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -34,10 +34,6 @@ 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") @@ -574,11 +570,6 @@ 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. @@ -588,23 +579,13 @@ func (g *GithubClient) MergePull(pull models.PullRequest, pullOptions models.Pul if err != nil { return errors.Wrap(err, "fetching repo info") } - protection, _, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner.GetLogin(), *repo.Name, pull.BaseBranch) - if err != nil { - if !errors.Is(err, github.ErrBranchNotProtected) && !isBranchProtectionNotAvailable(err) { - return errors.Wrap(err, "getting branch protection rules") - } - } - requireLinearHistory := false - if protection != nil { - requireLinearHistory = protection.GetRequireLinearHistory().Enabled - } const ( defaultMergeMethod = "merge" rebaseMergeMethod = "rebase" squashMergeMethod = "squash" ) method := defaultMergeMethod - if !repo.GetAllowMergeCommit() || requireLinearHistory { + if !repo.GetAllowMergeCommit() { if repo.GetAllowRebaseMerge() { method = rebaseMergeMethod } else if repo.GetAllowSquashMerge() { diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index ddc658aca8..c1c30f03d4 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -787,10 +787,6 @@ func TestGithubClient_MergePullHandlesError(t *testing.T) { defer r.Body.Close() // nolint: errcheck w.WriteHeader(c.code) w.Write([]byte(resp)) // nolint: errcheck - case "/api/v3/repos/runatlantis/atlantis/branches/master/protection": - w.WriteHeader(404) - w.Write([]byte("{\"message\":\"Branch not protected\"}")) // nolint: errcheck - return default: t.Errorf("got unexpected request at %q", r.RequestURI) http.Error(w, "not found", http.StatusNotFound) @@ -817,8 +813,7 @@ func TestGithubClient_MergePullHandlesError(t *testing.T) { Hostname: "github.com", }, }, - Num: 1, - BaseBranch: "master", + Num: 1, }, models.PullRequestOptions{ DeleteSourceBranchOnMerge: false, }) @@ -836,184 +831,40 @@ func TestGithubClient_MergePullHandlesError(t *testing.T) { // use that method func TestGithubClient_MergePullCorrectMethod(t *testing.T) { cases := map[string]struct { - allowMerge bool - allowRebase bool - allowSquash bool - requireLinearHistory bool - protectedBranch bool - protectionAvailable bool - expMethod string + allowMerge bool + allowRebase bool + allowSquash bool + expMethod string }{ "all true": { - allowMerge: true, - allowRebase: true, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: true, - expMethod: "merge", + allowMerge: true, + allowRebase: true, + allowSquash: true, + expMethod: "merge", }, "all false (edge case)": { - allowMerge: false, - allowRebase: false, - allowSquash: false, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: true, - expMethod: "merge", + allowMerge: false, + allowRebase: false, + allowSquash: false, + expMethod: "merge", }, "merge: false rebase: true squash: true": { - allowMerge: false, - allowRebase: true, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: true, - expMethod: "rebase", + allowMerge: false, + allowRebase: true, + allowSquash: true, + expMethod: "rebase", }, "merge: false rebase: false squash: true": { - allowMerge: false, - allowRebase: false, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: true, - expMethod: "squash", + allowMerge: false, + allowRebase: false, + allowSquash: true, + expMethod: "squash", }, "merge: false rebase: true squash: false": { - allowMerge: false, - allowRebase: true, - allowSquash: false, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: true, - expMethod: "rebase", - }, - "protected, all true, rlh: false": { - allowMerge: true, - allowRebase: true, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: true, - protectionAvailable: true, - expMethod: "merge", - }, - "protected, all false (edge case), rlh: false": { - allowMerge: false, - allowRebase: false, - allowSquash: false, - requireLinearHistory: false, - protectedBranch: true, - protectionAvailable: true, - expMethod: "merge", - }, - "protected, merge: false rebase: true squash: true, rlh: false": { - allowMerge: false, - allowRebase: true, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: true, - protectionAvailable: true, - expMethod: "rebase", - }, - "protected, merge: false rebase: false squash: true, rlh: false": { - allowMerge: false, - allowRebase: false, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: true, - protectionAvailable: true, - expMethod: "squash", - }, - "protected, merge: false rebase: true squash: false, rlh: false": { - allowMerge: false, - allowRebase: true, - allowSquash: false, - requireLinearHistory: false, - protectedBranch: true, - protectionAvailable: true, - expMethod: "rebase", - }, - "protected, all true, rlh: true": { - allowMerge: true, - allowRebase: true, - allowSquash: true, - requireLinearHistory: true, - protectedBranch: true, - protectionAvailable: true, - expMethod: "rebase", - }, - "protected, all false (edge case), rlh: true": { - allowMerge: false, - allowRebase: false, - allowSquash: false, - requireLinearHistory: true, - protectedBranch: true, - protectionAvailable: true, - expMethod: "merge", - }, - "protected, merge: false rebase: true squash: true, rlh: true": { - allowMerge: false, - allowRebase: true, - allowSquash: true, - requireLinearHistory: true, - protectedBranch: true, - protectionAvailable: true, - expMethod: "rebase", - }, - "protected, merge: false rebase: false squash: true, rlh: true": { - allowMerge: false, - allowRebase: false, - allowSquash: true, - requireLinearHistory: true, - protectedBranch: true, - protectionAvailable: true, - expMethod: "squash", - }, - "protected, merge: false rebase: true squash: false, rlh: true": { - allowMerge: false, - allowRebase: true, - allowSquash: false, - requireLinearHistory: true, - protectedBranch: true, - protectionAvailable: true, - expMethod: "rebase", - }, - "protection not supported, all true": { - allowMerge: true, - allowRebase: true, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: false, - expMethod: "merge", - }, - "protection not supported, merge: false, rebase: true, squash: true": { - allowMerge: false, - allowRebase: true, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: false, - expMethod: "rebase", - }, - "protection not supported, merge: false, rebase: false, squash: true": { - allowMerge: false, - allowRebase: false, - allowSquash: true, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: false, - expMethod: "squash", - }, - "protection not supported, merge: false, rebase: true, squash: false": { - allowMerge: false, - allowRebase: true, - allowSquash: false, - requireLinearHistory: false, - protectedBranch: false, - protectionAvailable: false, - expMethod: "rebase", + allowMerge: false, + allowRebase: true, + allowSquash: false, + expMethod: "rebase", }, } @@ -1023,10 +874,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { // Modify response. jsBytes, err := os.ReadFile("testdata/github-repo.json") Ok(t, err) - rlhBytes, err := os.ReadFile("testdata/github-branch-protection-require-linear-history.json") - Ok(t, err) resp := string(jsBytes) - protected := string(rlhBytes) resp = strings.Replace(resp, `"allow_squash_merge": true`, fmt.Sprintf(`"allow_squash_merge": %t`, c.allowSquash), @@ -1039,10 +887,6 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { `"allow_rebase_merge": true`, fmt.Sprintf(`"allow_rebase_merge": %t`, c.allowRebase), -1) - protected = strings.Replace(protected, - `"enabled": true`, - fmt.Sprintf(`"enabled": %t`, c.requireLinearHistory), - -1) testServer := httptest.NewTLSServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -1050,17 +894,6 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { case "/api/v3/repos/runatlantis/atlantis": w.Write([]byte(resp)) // nolint: errcheck return - case "/api/v3/repos/runatlantis/atlantis/branches/master/protection": - if c.protectedBranch && c.protectionAvailable { - w.Write([]byte(protected)) // nolint: errcheck - } else if !c.protectedBranch && c.protectionAvailable { - w.WriteHeader(404) - w.Write([]byte("{\"message\":\"Branch not protected\"}")) // nolint: errcheck - } 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 - } - return case "/api/v3/repos/runatlantis/atlantis/pulls/1/merge": body, err := io.ReadAll(r.Body) Ok(t, err) @@ -1103,8 +936,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) { Hostname: "github.com", }, }, - Num: 1, - BaseBranch: "master", + Num: 1, }, models.PullRequestOptions{ DeleteSourceBranchOnMerge: false, }) diff --git a/server/events/vcs/testdata/github-branch-protection-require-linear-history.json b/server/events/vcs/testdata/github-branch-protection-require-linear-history.json deleted file mode 100644 index c4d4de34e4..0000000000 --- a/server/events/vcs/testdata/github-branch-protection-require-linear-history.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "url": "https://api.github.com/repos/octocat/Hello-World/branches/master/protection", - "required_linear_history": { - "enabled": true - } -} \ No newline at end of file