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(github): branch protection not supported #3276

Merged
merged 2 commits into from
Mar 30, 2023
Merged

fix(github): branch protection not supported #3276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6436180
fix: github branch protection not supported
tpolekhin Mar 29, 2023
14d822c
fix: typo protectionAvailable
tpolekhin Mar 29, 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
11 changes: 10 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 @@ -34,6 +34,10 @@ 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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a magic string and could be changed at any point on the API.

Is there an error code or boolean that we could check instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but I think this is the way of the GitHub API unfortunately.
I took inspiration from the go-github package and how they handle branch not protected errors.
https://github.com/google/go-github/blob/master/github/repos.go#L17
https://github.com/google/go-github/blob/master/github/repos.go#L2028-L2031
https://github.com/google/go-github/blob/master/github/repos.go#L1300-L1302

Response code is HTTP 403 which is not definitive as well :(

< HTTP/2 403
< server: GitHub.com
< date: Tue, 28 Mar 2023 17:06:56 GMT
< content-type: application/json; charset=utf-8
< content-length: 200
< x-oauth-scopes: repo
< x-accepted-oauth-scopes: repo
< github-authentication-token-expiration: 2023-04-04 17:05:36 UTC
< x-github-media-type: github.v3; format=json
< x-github-api-version-selected: 2022-11-28
< x-ratelimit-limit: 5000
< x-ratelimit-remaining: 4999
< x-ratelimit-reset: 1680026816
< x-ratelimit-used: 1
< x-ratelimit-resource: core
< access-control-expose-headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset
< access-control-allow-origin: *
< strict-transport-security: max-age=31536000; includeSubdomains; preload
< x-frame-options: deny
< x-content-type-options: nosniff
< x-xss-protection: 0
< referrer-policy: origin-when-cross-origin, strict-origin-when-cross-origin
< content-security-policy: default-src 'none'
< vary: Accept-Encoding, Accept, X-Requested-With
< x-github-request-id: D5A2:06BA:5BE84E2:5D62843:64231EAF
<
{
  "message": "Upgrade to GitHub Pro or make this repository public to enable this feature.",
  "documentation_url": "https://docs.github.com/rest/branches/branch-protection#get-branch-protection"
}

Copy link
Member

@nitrocode nitrocode Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair enough. It's a long string tho and subject to change but i suppose we can iterate over it over time if it breaks.

If we do add this logic, id feel better with a shorter string and a regex. That's probably overkill.

@runatlantis/maintainers thoughts on this magic string solution and root issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with the string, I found it a bit shocking the API does not return something more programmatic like a bool or something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can list all protected branches for a repo and check current

https://github.com/google/go-github/blob/master/github/repos.go#L1214

Not sure if it's an efficient way of dealing with this though


var (
clientMutationID = githubv4.NewString("atlantis")
pullRequestDismissalMessage = *githubv4.NewString("Dismissing reviews because of plan changes")
Expand Down Expand Up @@ -570,6 +574,11 @@ 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 @@ -581,7 +590,7 @@ func (g *GithubClient) MergePull(pull models.PullRequest, pullOptions models.Pul
}
protection, _, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner.GetLogin(), *repo.Name, pull.BaseBranch)
if err != nil {
if !errors.Is(err, github.ErrBranchNotProtected) {
if !errors.Is(err, github.ErrBranchNotProtected) && !isBranchProtectionNotAvailable(err) {
return errors.Wrap(err, "getting branch protection rules")
}
}
Expand Down
59 changes: 57 additions & 2 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 @@ -841,6 +841,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash bool
requireLinearHistory bool
protectedBranch bool
protectionAvailable bool
expMethod string
}{
"all true": {
Expand All @@ -849,6 +850,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: false,
protectedBranch: false,
protectionAvailable: true,
expMethod: "merge",
},
"all false (edge case)": {
Expand All @@ -857,6 +859,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: false,
requireLinearHistory: false,
protectedBranch: false,
protectionAvailable: true,
expMethod: "merge",
},
"merge: false rebase: true squash: true": {
Expand All @@ -865,6 +868,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: false,
protectedBranch: false,
protectionAvailable: true,
expMethod: "rebase",
},
"merge: false rebase: false squash: true": {
Expand All @@ -873,6 +877,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: false,
protectedBranch: false,
protectionAvailable: true,
expMethod: "squash",
},
"merge: false rebase: true squash: false": {
Expand All @@ -881,6 +886,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: false,
requireLinearHistory: false,
protectedBranch: false,
protectionAvailable: true,
expMethod: "rebase",
},
"protected, all true, rlh: false": {
Expand All @@ -889,6 +895,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: false,
protectedBranch: true,
protectionAvailable: true,
expMethod: "merge",
},
"protected, all false (edge case), rlh: false": {
Expand All @@ -897,6 +904,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: false,
requireLinearHistory: false,
protectedBranch: true,
protectionAvailable: true,
expMethod: "merge",
},
"protected, merge: false rebase: true squash: true, rlh: false": {
Expand All @@ -905,6 +913,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: false,
protectedBranch: true,
protectionAvailable: true,
expMethod: "rebase",
},
"protected, merge: false rebase: false squash: true, rlh: false": {
Expand All @@ -913,6 +922,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: false,
protectedBranch: true,
protectionAvailable: true,
expMethod: "squash",
},
"protected, merge: false rebase: true squash: false, rlh: false": {
Expand All @@ -921,6 +931,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: false,
requireLinearHistory: false,
protectedBranch: true,
protectionAvailable: true,
expMethod: "rebase",
},
"protected, all true, rlh: true": {
Expand All @@ -929,6 +940,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: true,
protectedBranch: true,
protectionAvailable: true,
expMethod: "rebase",
},
"protected, all false (edge case), rlh: true": {
Expand All @@ -937,6 +949,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: false,
requireLinearHistory: true,
protectedBranch: true,
protectionAvailable: true,
expMethod: "merge",
},
"protected, merge: false rebase: true squash: true, rlh: true": {
Expand All @@ -945,6 +958,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: true,
protectedBranch: true,
protectionAvailable: true,
expMethod: "rebase",
},
"protected, merge: false rebase: false squash: true, rlh: true": {
Expand All @@ -953,6 +967,7 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
allowSquash: true,
requireLinearHistory: true,
protectedBranch: true,
protectionAvailable: true,
expMethod: "squash",
},
"protected, merge: false rebase: true squash: false, rlh: true": {
Expand All @@ -961,6 +976,43 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
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",
},
}
Expand Down Expand Up @@ -999,11 +1051,14 @@ func TestGithubClient_MergePullCorrectMethod(t *testing.T) {
w.Write([]byte(resp)) // nolint: errcheck
return
case "/api/v3/repos/runatlantis/atlantis/branches/master/protection":
if c.protectedBranch {
if c.protectedBranch && c.protectionAvailable {
w.Write([]byte(protected)) // nolint: errcheck
} else {
} 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":
Expand Down