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

fix: revert branch protection feature causing breaking change #3321

Merged
merged 1 commit into from
Apr 11, 2023
Merged

fix: revert branch protection feature causing breaking change #3321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 1 addition & 20 deletions server/events/vcs/github_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop

This file was deleted.