Skip to content

Commit

Permalink
fix: allow Require Linear History when selecting merge method (#3211)
Browse files Browse the repository at this point in the history 
* fix: account for Require Linear History when selecting merge method

* fix logic and add tests

* add nolint hints for w.Write

---------

Co-authored-by: nitrocode <[email protected]>
  • Loading branch information
@tpolekhin @nitrocode
tpolekhin and nitrocode authored Mar 18, 2023
1 parent 47f0258 commit 7a33828
Show file tree
Hide file tree
Showing 3 changed files with 156 additions and 27 deletions.
12 changes: 11 additions & 1 deletion server/events/vcs/github_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,13 +579,23 @@ 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) {
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() {
if !repo.GetAllowMergeCommit() || requireLinearHistory {
if repo.GetAllowRebaseMerge() {
method = rebaseMergeMethod
} else if repo.GetAllowSquashMerge() {
Expand Down
165 changes: 139 additions & 26 deletions server/events/vcs/github_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,10 @@ 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 @@ -813,7 +817,8 @@ func TestGithubClient_MergePullHandlesError(t *testing.T) {
Hostname: "github.com",
},
},
Num: 1,
Num: 1,
BaseBranch: "master",
}, models.PullRequestOptions{
DeleteSourceBranchOnMerge: false,
})
Expand All @@ -831,40 +836,132 @@ 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
expMethod string
allowMerge bool
allowRebase bool
allowSquash bool
requireLinearHistory bool
protectedBranch bool
expMethod string
}{
"all true": {
allowMerge: true,
allowRebase: true,
allowSquash: true,
expMethod: "merge",
allowMerge: true,
allowRebase: true,
allowSquash: true,
requireLinearHistory: false,
protectedBranch: false,
expMethod: "merge",
},
"all false (edge case)": {
allowMerge: false,
allowRebase: false,
allowSquash: false,
expMethod: "merge",
allowMerge: false,
allowRebase: false,
allowSquash: false,
requireLinearHistory: false,
protectedBranch: false,
expMethod: "merge",
},
"merge: false rebase: true squash: true": {
allowMerge: false,
allowRebase: true,
allowSquash: true,
expMethod: "rebase",
allowMerge: false,
allowRebase: true,
allowSquash: true,
requireLinearHistory: false,
protectedBranch: false,
expMethod: "rebase",
},
"merge: false rebase: false squash: true": {
allowMerge: false,
allowRebase: false,
allowSquash: true,
expMethod: "squash",
allowMerge: false,
allowRebase: false,
allowSquash: true,
requireLinearHistory: false,
protectedBranch: false,
expMethod: "squash",
},
"merge: false rebase: true squash: false": {
allowMerge: false,
allowRebase: true,
allowSquash: false,
expMethod: "rebase",
allowMerge: false,
allowRebase: true,
allowSquash: false,
requireLinearHistory: false,
protectedBranch: false,
expMethod: "rebase",
},
"protected, all true, rlh: false": {
allowMerge: true,
allowRebase: true,
allowSquash: true,
requireLinearHistory: false,
protectedBranch: true,
expMethod: "merge",
},
"protected, all false (edge case), rlh: false": {
allowMerge: false,
allowRebase: false,
allowSquash: false,
requireLinearHistory: false,
protectedBranch: true,
expMethod: "merge",
},
"protected, merge: false rebase: true squash: true, rlh: false": {
allowMerge: false,
allowRebase: true,
allowSquash: true,
requireLinearHistory: false,
protectedBranch: true,
expMethod: "rebase",
},
"protected, merge: false rebase: false squash: true, rlh: false": {
allowMerge: false,
allowRebase: false,
allowSquash: true,
requireLinearHistory: false,
protectedBranch: true,
expMethod: "squash",
},
"protected, merge: false rebase: true squash: false, rlh: false": {
allowMerge: false,
allowRebase: true,
allowSquash: false,
requireLinearHistory: false,
protectedBranch: true,
expMethod: "rebase",
},
"protected, all true, rlh: true": {
allowMerge: true,
allowRebase: true,
allowSquash: true,
requireLinearHistory: true,
protectedBranch: true,
expMethod: "rebase",
},
"protected, all false (edge case), rlh: true": {
allowMerge: false,
allowRebase: false,
allowSquash: false,
requireLinearHistory: true,
protectedBranch: true,
expMethod: "merge",
},
"protected, merge: false rebase: true squash: true, rlh: true": {
allowMerge: false,
allowRebase: true,
allowSquash: true,
requireLinearHistory: true,
protectedBranch: true,
expMethod: "rebase",
},
"protected, merge: false rebase: false squash: true, rlh: true": {
allowMerge: false,
allowRebase: false,
allowSquash: true,
requireLinearHistory: true,
protectedBranch: true,
expMethod: "squash",
},
"protected, merge: false rebase: true squash: false, rlh: true": {
allowMerge: false,
allowRebase: true,
allowSquash: false,
requireLinearHistory: true,
protectedBranch: true,
expMethod: "rebase",
},
}

Expand All @@ -874,7 +971,10 @@ 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 @@ -887,13 +987,25 @@ 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 {
w.Write([]byte(protected)) // nolint: errcheck
} else {
w.WriteHeader(404)
w.Write([]byte("{\"message\":\"Branch not protected\"}")) // 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 @@ -936,7 +1048,8 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
Hostname: "github.com",
},
},
Num: 1,
Num: 1,
BaseBranch: "master",
}, models.PullRequestOptions{
DeleteSourceBranchOnMerge: false,
})
Expand Down
6 changes: 6 additions & 0 deletions server/events/vcs/testdata/github-branch-protection-require-linear-history.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"url": "https://api.github.com/repos/octocat/Hello-World/branches/master/protection",
"required_linear_history": {
"enabled": true
}
}

0 comments on commit 7a33828

Please sign in to comment.