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: allow Require Linear History when selecting merge method #3211

Merged
merged 5 commits into from
Mar 18, 2023
Merged

fix: allow Require Linear History when selecting merge method #3211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f2de5e3
fix: account for Require Linear History when selecting merge method
tpolekhin Mar 15, 2023
d62d733
fix logic and add tests
tpolekhin Mar 15, 2023
ae657b9
add nolint hints for w.Write
tpolekhin Mar 16, 2023
c014eb3
Merge branch 'main' into fix/github-merge-require-linear-history
nitrocode Mar 17, 2023
c019d8b
Merge branch 'main' into fix/github-merge-require-linear-history
nitrocode Mar 17, 2023
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
12 changes: 11 additions & 1 deletion server/events/vcs/github_client.go
View file
Open in desktop
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)
nitrocode marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
if !errors.Is(err, github.ErrBranchNotProtected) {
return errors.Wrap(err, "getting branch protection rules")
nitrocode marked this conversation as resolved.
Show resolved Hide resolved
}
}
requireLinearHistory := false
tpolekhin marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
}
}
nitrocode marked this conversation as resolved.
Show resolved Hide resolved