Mergeable status is always false when branch GH protection doesn't require reviewers

[stasostrovskyi]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
• If you are interested in working on this issue or have submitted a pull request, please leave a comment.

---

Overview of the Issue

Using new (awesome!) feature that allows to skip atlantis/apply in atlantis mergeable requirement (#2436), we've stumbled upon a corner case - when branch protection doesn't have required reviewers, Atlantis mistakenly reports that PR is not mergeable even though it clearly is. The reason for the issue is that reviewDecision in Pull Request GraphQL quesry would become null instead of APPROVED. Simple fix would be to treat null as success here - https://github.com/runatlantis/atlantis/blob/master/server/events/vcs/github_client.go#L400.

Context: this can happen if some other tool is used to decide who should do the review, in our case its https://github.com/palantir/policy-bot.

Reproduction Steps

1. Run atlantis with --gh-allow-mergeable-bypass-apply
2. Set apply_requirements to [mergeable] in atlantis.yaml
3. Set up branch protection so it doesn't require review approvals or Code Owners review
4. Create PR and get a plan
5. Run atlantis apply

Logs

caller: events/instrumented_project_command_runner.go:49, json: {…}, level: error, msg: Failure running apply operation: Pull request must be mergeable before running apply., stacktrace: github.com/runatlantis/atlantis/server/events.RunAndEmitStats github.com/runatlantis/atlantis/server/events/ins…

Environment details

Atlantis version: v0.19.9-pre.2022082
Atlantis flags: --gh-allow-mergeable-bypass-apply

Additional Context

[jamengual]

@rayterrill did you test this case?, or could you test and see if is a issue

[stasostrovskyi]

Also, just to add about this new feature, it seems to heavily suffer from #2285. We use atlantis with a github app for a long time, but we only started to see the issue with 401s after enabling --gh-allow-mergeable-bypass-apply

[rayterrill]

@rayterrill did you test this case?, or could you test and see if is a issue

Will test this tomorrow. I def tested around this case, but might've missed something.

[rayterrill]

Also, just to add about this new feature, it seems to heavily suffer from #2285. We use atlantis with a github app for a long time, but we only started to see the issue with 401s after enabling --gh-allow-mergeable-bypass-apply

@stasostrovskyi any chance you can flip that into debug mode and see what API call is failing? I have a feeling it's this: GET /repos/:owner/:repo/branches/:branch/protection - which needs an additional permission, 'Read-only' on the 'Administration' repo permission for the github app (we use github apps as well). In debug mode if it's the same thing it was pretty clear what this was the issue, and the additional permission cleared that up.

I'm actually wondering if this missing permission is the cause of this issue - would be good to confirm that (there's new logic to look at requires checks and only include those in the calculation for whether it's mergeable). If it is, it's probably my bad for not noting the need for the additional permission. This was my first contrib and I'm not sure how to go about doing that.

[stasostrovskyi]

Hi @rayterrill! Yes, we have added 'Administration' permission for the github app, but it didn't help. If that would be an issue with query/permissions we would see https://github.com/runatlantis/atlantis/blob/master/server/events/vcs/github_client.go#L397 error in the logs. But instead, we don't see any errors in the log (except one mentioned in the description).

Let me clarify our setup and what happens in graphql. So here is our branch protection setup:

And then we do this graphql query:

gh api graphql -F owner='org-name' -F name='repository-name' -f query='
      query($name: String!, $owner: String!) {
           repository(owner: $owner, name: $name) {
               pullRequest(number: PR-number) {
                   reviewDecision
               }
          }
      }'

And api returns:

{
  "data": {
    "repository": {
      "pullRequest": {
        "reviewDecision": null
      }
    }
  }
}

It's just a case of bad documentation from GitHub side that this situation can happen (or maybe even a bug in their API).

P.S. Also, if you do use GitHub App how come you don't get 401 every now and then when doing graphql queries? I haven't seen anywhere in the code that the token is being refreshed...

[rayterrill]

It looks like we are indeed seeing the 401 - I just hadn't looked at the logs in the container I have this running in since it's been running for a bit. Looking at the code, it does indeed look like it's because atlantis is building that graphql client with the token once.

I'll see if I can look at both, just not sure the best way to handle the token part of the graphql. It looks like the call to get an access token for an app should return an expire time (https://docs.github.com/en/rest/apps/apps#create-an-installation-access-token-for-an-app), we could use that, or we could just get a new token every time, not sure if there's a best practice for the project here.

[stasostrovskyi]

I've already created a PR for the issue raised here - #2470. It's mostly about adding unit test to verify that new code works as expected. Would be awesome, if you can take a look at the token issue, otherwise, I can do it next week.

Atlantis already has a code for refreshing git token (

atlantis/server/events/github_app_working_dir.go

Lines 28 to 31 in 96a25bd

	token, err := g.Credentials.GetToken()
	if err != nil {
	return "", false, errors.Wrap(err, "getting github token")
	}

), so I guess it would make sense to refresh a token before any graphql query maybe?

[jamengual]

the 401 is not related directly to the issue posted here but it seems to be related to the token refresh and rate limiting being differently in Github apps and webhooks so that needs to be taken into consideration when creating a solution to refresh the token.

[jamengual]

@rayterrill is that something you want to work on it?

[rayterrill]

Yep @jamengual I can take a look at it - been digging into it this morning to fully understand how it's all working.

[jamengual]

awesome, thanks @rayterrill

[jamengual]

401 can be rate limiting by using the GitHub app instead of the webhook. there is different rate limits per each implementation.

…

On Fri, Aug 26, 2022 at 12:49 AM Stas Ostrovskyi ***@***.***> wrote: Hi @rayterrill <https://github.com/rayterrill>! Yes, we have added 'Administration' permission for the github app, but it didn't help. If that would be an issue with query/permissions we would see https://github.com/runatlantis/atlantis/blob/master/server/events/vcs/github_client.go#L397 error in the logs. But instead, we don't see any errors in the log (except one mentioned in the description). Let me clarify our setup and what happens in graphql. So here is our branch protection setup: [image: image] <https://user-images.githubusercontent.com/7407450/186844907-f1c16671-cad9-4226-8941-b46cf29e7b5e.png> And then we do this graphql query: gh api graphql -F owner='org-name' -F name='repository-name' -f query=' query($name: String!, $owner: String!) { repository(owner: $owner, name: $name) { pullRequest(number: PR-number) { reviewDecision } } }' And api returns: { "data": { "repository": { "pullRequest": { "reviewDecision": null } } } } It's just a case of bad documentation from GitHub side that this situation can happen (or maybe even a bug in their API). P.S. Also, if you do use GitHub App how come you don't get 401 every now and then when doing graphql queries? I haven't seen anywhere in the code that the token is being refreshed... — Reply to this email directly, view it on GitHub <#2469 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERB3IHTE4XWUJIHR6ILV3BZJRANCNFSM57PHZMEQ> . You are receiving this because you commented.Message ID: ***@***.***>