Skip to content

Commit

Permalink
fix: revert branch protection feature causing breaking change
Browse files Browse the repository at this point in the history 
Reverts "fix(github): branch protection not supported (#3276)"
Reverts "fix: allow Require Linear History when selecting merge method (#3211)"
Closes #3320
  • Loading branch information
@GenPage
GenPage committed Apr 11, 2023
1 parent 763f017 commit 34ad9e9
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 220 deletions.
21 changes: 1 addition & 20 deletions server/events/vcs/github_client.go
View file
Expand Up @@ -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")
Expand Down Expand Up @@ -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.
Expand All @@ -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() {
Expand Down
220 changes: 26 additions & 194 deletions server/events/vcs/github_client_test.go
View file
Expand Up @@ -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)
Expand All @@ -817,8 +813,7 @@ func TestGithubClient_MergePullHandlesError(t *testing.T) {
Hostname: "github.com",
},
},
Num: 1,
BaseBranch: "master",
Num: 1,
}, models.PullRequestOptions{
DeleteSourceBranchOnMerge: false,
})
Expand All @@ -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",
},
}

Expand All @@ -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),
Expand All @@ -1039,28 +887,13 @@ 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) {
switch r.RequestURI {
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)
Expand Down Expand Up @@ -1103,8 +936,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
Hostname: "github.com",
},
},
Num: 1,
BaseBranch: "master",
Num: 1,
}, models.PullRequestOptions{
DeleteSourceBranchOnMerge: false,
})
Expand Down
6 changes: 0 additions & 6 deletions server/events/vcs/testdata/github-branch-protection-require-linear-history.json
View file

This file was deleted.

0 comments on commit 34ad9e9

Please sign in to comment.