fix(github): branch protection not supported #3276

tpolekhin · 2023-03-29T11:35:36Z

what

fix github branch protection not supported

why

Correctly handle GitHub API error when branch protection is not available in Free Private repositories

tests

I have tested my changes by running tests in VSCode
I have tested my changes by running make test
I have tested my changes by running make build

references

closes Automerge fails on github free accounts in 0.23.3 due to branch protection api error #3268

nitrocode · 2023-03-29T13:33:02Z

server/events/vcs/github_client.go

@@ -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."


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?

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" }

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?

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.

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

server/events/vcs/github_client_test.go

nitrocode · 2023-03-29T14:22:26Z

Thank you @tpolekhin for the pr. I do not think this closes issue #3268 completely until we can also check for 404 not found on the branch protection endpoint (due to non admin perms).

Is that something you can also tackle within this pr? Or should be done in a follow up?

tpolekhin · 2023-03-29T14:27:03Z

Thank you @tpolekhin for the pr. I do not think this closes issue #3268 completely until we can also check for 404 not found on the branch protection endpoint (due to non admin perms).

Is that something you can also tackle within this pr? Or should be done in a follow up?

that is actually a tricky one, because 404 Not Found could mean 2 things:

Branch is not protected
Not enough permissions to get branch protection

It doesn't feel right to treat 2 as if branch is not protected because it can lead to errors later down the chain when incorrect merge method will be chosen.

nitrocode · 2023-03-29T14:49:41Z

I was thinking that if a 404 was returned from the api, then we could have an info message after the stack trace

404 not found. Please check if branch protections are set and if atlantis has admin read only access to the repo

Or similar. This way people would be more inclined to try one of those methods before reaching out to a github issue

nitrocode · 2023-03-30T02:42:39Z

@tpolekhin can you confirm if works end to end in your own setup? Even though it's a simple fix and contains tests, id like to double check. Sometimes when we ask this question, people try it out and find other problems with the pr and then can catch it in time

tpolekhin · 2023-03-30T11:26:32Z

@nitrocode fair. I did run a test on a free private repository without a protected branch without issues.
PR was merged automatically.

Apply and merge logs:

{"level":"info","ts":"2023-03-30T11:24:42.197Z","caller":"events/events_controller.go:545","msg":"parsed comment as command=\"apply\" verbose=false dir=\"\" workspace=\"\" project=\"\" flags=\"\"","json":{"gh-request-id":"X-Github-Delivery=76a5c5e0-ceed-11ed-98de-bc6831dbe53f"}}
{"level":"info","ts":"2023-03-30T11:24:43.471Z","caller":"terraform/terraform_client.go:316","msg":"Cannot determine which version to use from terraform configuration, detected 0 possibilities.","json":{"repo":"tpolekhin/atlantis-test-terraform","pull":"6"}}
{"level":"info","ts":"2023-03-30T11:24:43.730Z","caller":"runtime/apply_step_runner.go:39","msg":"starting apply","json":{"repo":"tpolekhin/atlantis-test-terraform","pull":"6"}}
{"level":"info","ts":"2023-03-30T11:24:43.965Z","caller":"models/shell_command_runner.go:156","msg":"successfully ran \"/usr/local/bin/terraform apply -input=false \\\"/home/atlantis/.atlantis/repos/tpolekhin/atlantis-test-terraform/6/default/default.tfplan\\\"\" in \"/home/atlantis/.atlantis/repos/tpolekhin/atlantis-test-terraform/6/default\"","json":{"repo":"tpolekhin/atlantis-test-terraform","pull":"6"}}
{"level":"info","ts":"2023-03-30T11:24:43.966Z","caller":"runtime/apply_step_runner.go:58","msg":"apply successful, deleting planfile","json":{"repo":"tpolekhin/atlantis-test-terraform","pull":"6"}}
{"level":"info","ts":"2023-03-30T11:24:44.301Z","caller":"events/instrumented_project_command_runner.go:87","msg":"apply success. output available at: https://github.com/tpolekhin/atlantis-test-terraform/pull/6","json":{"repo":"tpolekhin/atlantis-test-terraform","pull":"6"}}
{"level":"info","ts":"2023-03-30T11:24:46.541Z","caller":"events/automerger.go:32","msg":"automerging pull request","json":{"repo":"tpolekhin/atlantis-test-terraform","pull":"6"}}
{"level":"info","ts":"2023-03-30T11:24:49.134Z","caller":"events/instrumented_pull_closed_executor.go:45","msg":"Initiating cleanup of pull data.","json":{"repository":"tpolekhin/atlantis-test-terraform","pull-num":"6"}}
{"level":"info","ts":"2023-03-30T11:24:50.104Z","caller":"events/events_controller.go:473","msg":"deleted locks and workspace for repo tpolekhin/atlantis-test-terraform, pull 6","json":{"gh-request-id":"X-Github-Delivery=7ac53930-ceed-11ed-94bb-ce472f3c8467"}}

nitrocode · 2023-03-30T16:33:15Z

Thank you @tpolekhin !

This reverts commit ebc06c1. Revert "fix: allow `Require Linear History` when selecting merge method (#3211)" This reverts commit 7a33828.

Reverts "fix(github): branch protection not supported (#3276)" Reverts "fix: allow Require Linear History when selecting merge method (#3211)" Closes #3320

nitrocode · 2023-04-12T03:33:36Z

@tpolekhin this was reverted due to a breaking change. See the related pr #3321

* fix: github branch protection not supported * fix: typo protectionAvailable

Reverts "fix(github): branch protection not supported (#3276)" Reverts "fix: allow Require Linear History when selecting merge method (#3211)" Closes #3320

* fix: github branch protection not supported * fix: typo protectionAvailable

Reverts "fix(github): branch protection not supported (runatlantis#3276)" Reverts "fix: allow Require Linear History when selecting merge method (runatlantis#3211)" Closes runatlantis#3320

* fix: github branch protection not supported * fix: typo protectionAvailable

Reverts "fix(github): branch protection not supported (runatlantis#3276)" Reverts "fix: allow Require Linear History when selecting merge method (runatlantis#3211)" Closes runatlantis#3320

fix: github branch protection not supported

6436180

tpolekhin requested a review from a team as a code owner March 29, 2023 11:35

github-actions bot added go Pull requests that update Go code provider/github labels Mar 29, 2023

nitrocode reviewed Mar 29, 2023

View reviewed changes

server/events/vcs/github_client_test.go Outdated Show resolved Hide resolved

fix: typo protectionAvailable

14d822c

nitrocode approved these changes Mar 29, 2023

View reviewed changes

nitrocode added this to the v0.23.4 milestone Mar 29, 2023

nitrocode changed the title ~~fix: github branch protection not supported~~ fix(github): branch protection not supported Mar 30, 2023

nitrocode merged commit ebc06c1 into runatlantis:main Mar 30, 2023

GenPage added a commit that referenced this pull request Apr 11, 2023

Revert "fix(github): branch protection not supported (#3276)"

6dabc45

This reverts commit ebc06c1. Revert "fix: allow `Require Linear History` when selecting merge method (#3211)" This reverts commit 7a33828.

GenPage mentioned this pull request Apr 11, 2023

fix: revert branch protection feature causing breaking change #3321

Merged

nitrocode pushed a commit that referenced this pull request May 5, 2023

fix(github): branch protection not supported (#3276)

f35becc

* fix: github branch protection not supported * fix: typo protectionAvailable

albertorm95 mentioned this pull request May 19, 2023

fix(metrics): add missing stat metric scopes #3416

Merged

ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024

fix(github): branch protection not supported (runatlantis#3276)

1091cc1

* fix: github branch protection not supported * fix: typo protectionAvailable

ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024

fix(github): branch protection not supported (runatlantis#3276)

c14bdcb

* fix: github branch protection not supported * fix: typo protectionAvailable

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(github): branch protection not supported #3276

fix(github): branch protection not supported #3276

tpolekhin commented Mar 29, 2023 •

edited by nitrocode

Loading

nitrocode Mar 29, 2023

tpolekhin Mar 29, 2023

nitrocode Mar 29, 2023 •

edited

Loading

jamengual Mar 29, 2023

tpolekhin Mar 29, 2023

nitrocode commented Mar 29, 2023 •

edited

Loading

tpolekhin commented Mar 29, 2023

nitrocode commented Mar 29, 2023

nitrocode commented Mar 30, 2023

tpolekhin commented Mar 30, 2023 •

edited

Loading

nitrocode commented Mar 30, 2023

nitrocode commented Apr 12, 2023

fix(github): branch protection not supported #3276

fix(github): branch protection not supported #3276

Conversation

tpolekhin commented Mar 29, 2023 • edited by nitrocode Loading

what

why

tests

references

nitrocode Mar 29, 2023

Choose a reason for hiding this comment

tpolekhin Mar 29, 2023

Choose a reason for hiding this comment

nitrocode Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

jamengual Mar 29, 2023

Choose a reason for hiding this comment

tpolekhin Mar 29, 2023

Choose a reason for hiding this comment

nitrocode commented Mar 29, 2023 • edited Loading

tpolekhin commented Mar 29, 2023

nitrocode commented Mar 29, 2023

nitrocode commented Mar 30, 2023

tpolekhin commented Mar 30, 2023 • edited Loading

nitrocode commented Mar 30, 2023

nitrocode commented Apr 12, 2023

tpolekhin commented Mar 29, 2023 •

edited by nitrocode

Loading

nitrocode Mar 29, 2023 •

edited

Loading

nitrocode commented Mar 29, 2023 •

edited

Loading

tpolekhin commented Mar 30, 2023 •

edited

Loading