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.

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

"GitHub Allow Mergable Bypass Apply" Broken for Fork PRs #2601

Closed
alexlo03 opened this issue Oct 19, 2022 · 12 comments · Fixed by #3620
Closed

"GitHub Allow Mergable Bypass Apply" Broken for Fork PRs #2601

alexlo03 opened this issue Oct 19, 2022 · 12 comments · Fixed by #3620
Labels
bug Something isn't working help wanted Good feature for contributors

Comments

@alexlo03
Copy link

alexlo03 commented Oct 19, 2022

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

--gh-allow-mergeable-bypass-apply as described here: https://www.runatlantis.io/docs/apply-requirements.html#github

Seems broken for FORK PRs but works fine with BRANCH PRs.

Reproduction Steps

Repo has atlantis/apply as a branch protection mergeable requirement. Open up a branch PR and have it work correctly. Open up a Fork PR and see warning and error in the logs (and the PR is not able to be merged until the branch protection is edited).

Logs

Logs

The following warning then error come in the logs:

Error

Unable to check pull mergeable status, error: getting pull request status: getting combined status: GET https://api.github.com/repos/ORG/REPO/commits/BRANCH_NAME_FROM_FORK/status: 404 Ref not found []

Stacktrace

github.com/runatlantis/atlantis/server/events/vcs.(*InstrumentedClient).PullIsMergeable
	github.com/runatlantis/atlantis/server/events/vcs/instrumented_client.go:183
github.com/runatlantis/atlantis/server/events/vcs.(*ClientProxy).PullIsMergeable
	github.com/runatlantis/atlantis/server/events/vcs/proxy.go:72
github.com/runatlantis/atlantis/server/events/vcs.(*pullReqStatusFetcher).FetchPullStatus
	github.com/runatlantis/atlantis/server/events/vcs/pull_status_fetcher.go:28
github.com/runatlantis/atlantis/server/events.(*ApplyCommandRunner).Run
	github.com/runatlantis/atlantis/server/events/apply_command_runner.go:108
github.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunCommentCommand
	github.com/runatlantis/atlantis/server/events/command_runner.go:296

Warning

unable to get pull request status: fetching mergeability status for repo: ORG/REPO, and pull number: 573: getting pull request status: getting combined status: GET https://api.github.com/repos/ORG/REPO/commits/BRANCH_NAME_FROM_FORK/status: 404 Ref not found []. Continuing with mergeable and approved assumed false

Stacktrace

github.com/runatlantis/atlantis/server/events.(*ApplyCommandRunner).Run
	github.com/runatlantis/atlantis/server/events/apply_command_runner.go:114
github.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunCommentCommand
	github.com/runatlantis/atlantis/server/events/command_runner.go:296

Environment details

  • Atlantis version: 0.19.9
  • Atlantis flags:
  • ENV ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY=true

Additional Context

I tried figuring out the Go code but I hit githubPR.GetMergeableState() which does not seem to be defined anywhere?

state := githubPR.GetMergeableState()

The GitHub API it is failing on is https://docs.github.com/en/rest/commits/statuses#get-the-combined-status-for-a-specific-reference
The issue is that the commit ref does not exist in the repository the PR is against, but is coming from the fork.

Thanks

@alexlo03 alexlo03 added the bug Something isn't working label Oct 19, 2022
@zepeng811
Copy link
Contributor

I think I had the same issue when I tried to use it, but did not identify it was a fork vs branch problem. Now going back looking at the problem PR it was indeed created from a fork.

To give more information, when I tried to atlantis apply on a forked PR that is approved and already satisfy the merge requirement indicated by GitHub, Atlantis responded the following:

Ran Apply for project: <project_name> dir: <dir_name> workspace: default

Apply Failed: Pull request must be approved by at least one person other than the author before running apply.

not sure if others are seeing the same or a different error message.

@zepeng811
Copy link
Contributor

and FYI #2436 is the PR that added this feature.

@ayk33
Copy link

ayk33 commented Oct 20, 2022

I can confirm this issue is happening on the latest version of Atlantis v0.20.1

@roulettedares
Copy link
Contributor

pain

@ttretau
Copy link
Contributor

ttretau commented May 24, 2023

I was also able to reproduce this issue on a fork PR. I am working on a fix and I plan to create a PR.

@stasostrovskyi
Copy link
Contributor

You may want to try configuring Repositry Rules instead of fixing this. Make a rule that requires atlantis/apply and give atlantis user/app permission to bypass that rule. Then, you should be able to drop --gh-allow-mergable-bypass-apply.

@alexlo03
Copy link
Author

You may want to try configuring Repositry Rules instead of fixing this. Make a rule that requires atlantis/apply and give atlantis user/app permission to bypass that rule. Then, you should be able to drop --gh-allow-mergable-bypass-apply.

If Atlantis requires that the PR is "meragable" then this will not work around the issue. https://www.runatlantis.io/docs/command-requirements.html#mergeable

Atlantis may have the power to merge through the check, but it won't.

@ttretau
Copy link
Contributor

ttretau commented May 25, 2023

I assume the idea would be to remove the atlantis/apply as branch protection and move it to a Repository Rule.
By that the other "mergeable" requirements could remain and you would not need to activate the feature flag.

In my case this is not an option due to missing Repository Rule feature in the used Github Enterprise version.

@stasostrovskyi
Copy link
Contributor

Yes, GitHub Enterprise is a good reason to fix this one :) Just FYI for everyone here, whole bypass-atlantis-apply feature is more of a hack that only works for a specific use case (although pretty common) and only checks whether reviews are correct and status checks are passed. There are many settings in branch protection that are not covered by this feature and that's where Repository Rules should eventually come in play, instead of atlantis re-creating github logic.

@alexlo03
Copy link
Author

I see- I missed the new https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets feature.

The proposal is:

  1. Branch protection would not include atlantis/apply
  2. A Repository Rule "not-mergable-except-atlantis" would
    A) exempt the Atlantis User/App
    B) Require atlantis/apply

So everyone would be blocked by the rule except Atlantis.

@stasostrovskyi
Copy link
Contributor

Yes, and also Repository Rules will eventually replace branch protection entirely, which from the perspective of a default Atlantis flow shouldn't matter, but bypass-atlantis-apply will most probably break. However, I haven't tried the scenario with repository rules, because I'm waiting for GitHub to implement App bypass on the org level.

@alexlo03
Copy link
Author

alexlo03 commented Aug 4, 2023

I took a crack at this finally and don't think it works (or I am missing something)

I create Repository Rule "not-mergable-except-atlantis"
A) exempt the Atlantis User (we are not using app)
B) Require the check "atlantis/apply"

But then on a PR, the Atlantis mergable requirement is not met when I try to atlantis apply

If I take out "B" then the PR is mergable without apply.

@stasostrovskyi any thoughts?

==

Thanks to @ttretau for fixing this - looking forward to the release 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Good feature for contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants