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

Mergify & Branch Protection #312

Open
edahlseng opened this issue Dec 5, 2018 · 11 comments
Open

Mergify & Branch Protection #312

edahlseng opened this issue Dec 5, 2018 · 11 comments

Comments

@edahlseng
Copy link
Contributor

Is there a single status check that Mergify applies to a PR every time it determines that it's ready to merge? (Including both of PRs that it will merge, as well as ones that are blocked due to configuration changes?)

Right now our GitHub branch protections are preventing Mergify from making some automated merges when there are no reviewers. Nevertheless, we still want to prevent merging on other PRs until the proper number of reviewers has reviewed. This is something that Mergify tracks really well.... Therefore, we'd like to update our GitHub status checks to use a single Mergify status check in order to confirm when a PR is ready to merge or not, making Mergify the single source of truth.

@jd
Copy link
Member

jd commented Dec 5, 2018

@sileht will correct me if I'm wrong, but the only status check that Mergify reports is its summary (with more details available in the Checks tab).

IIUC you want to update your branch protection to allow merging only if Mergify says "OK" on a rule?

I don't think that's doable right now, but maybe it'd be doable if Mergify was reporting some rules as independent status checks. @sileht would have more insight on the doability of this.

@edahlseng
Copy link
Contributor Author

edahlseng commented Dec 5, 2018

@jd I'm actually just looking for something simpler than that – looking for a single Mergify status check that is applied whenever Mergify determines that all conditions have passed. Something that Mergify always uses, whether it's going to automatically merge the PR, or whether there's a Mergify configuration update (and thus Mergify won't actually merge that PR).

This single status check could then effectively become the one and only requirement to use in the GitHub branch protections, meaning that Mergify is the single source of truth for whether a PR is mergeable or not.

I'm having trouble determining whether there exists such a status code now (or whether it would need to be added) because looking at past PR's it looks like none of the Mergify status checks show in the checks details. I feel like I may have seen such a status check come in while watching the progress of a PR, however, so maybe it already exists?

@jd
Copy link
Member

jd commented Dec 6, 2018

Mergify always asks and trust behind about the mergeable state of a PR. If branch protections are in place, then the PR is reported as being blocked and Mergify won't merge it. If you set no conditions but enable branch protections, you'll actually see an error in Mergify stating that it can't merge because of branch protection. Mergify will retry the merge on the next event, until blocked is changed to mergeable.

Now, the problem is that often GitHub is out-of-sync. That means:

  1. The PR is blocked because Travis did not pass and branch protection is enabled
  2. Travis passes.
  3. GitHub sends an event to Mergify, e.g. Travis passes.
  4. Mergify treats the event and asks GitHub about the PR state: GitHub did not treat the event itself, and the PR is still marked as blocked.
  5. GitHub marks the PR as mergeable now that Travis has pass.
  6. Nothing happens. :(

@jd jd changed the title Mergify Approval Test Mergify & Branch Protection Dec 6, 2018
@muhlemmer
Copy link

We have a similar use case. I have an auto merge rule for "regular" pull requests, needing 2 reviews. Then a second rule which matches a group of "trusted" contributors that need only 1 review to merge.

Now, GitHub branch protection used to be set to 2, which makes mergify fail on the "1 review" rule. So I lower the branch protection to 1 review. Mergify works. However, now when a PR is submitted from a non-trusted user, a reviewer is able to approve and merge. (since a reviewer needs write access, he is able to do so).

The V1 mergify engine was working on branch protection by setting mergify/pr to pending. However, in the V2 this seems to be taken out. We just moved to the new syntax, hoping to set up the above.

If mergify - summary would give a pending state again, we could set up that state as protection rule and get rid of the review based branch protection. This would put the full control through .mergify.yml.

@muhlemmer
Copy link

Also tried to set mergify[bot] as the only user that is allowed to do merges, but Github does not recognize it as a username.

@edahlseng
Copy link
Contributor Author

@muhlemmer, the mergify/pr status check from V1 is a great example, I had forgotten about that.

@jd the issue you describe above, while frustrating, is a little bit different from what we're describing here. Just looking for a status check that represents Mergify's understanding of whether or not a PR is mergeable. What would it take to get a status check like that added? It seems like it wouldn't be too hard given that V1 had it?

@jd
Copy link
Member

jd commented Dec 14, 2018

@edahlseng I think that's what @sileht's doing in #324

@edahlseng
Copy link
Contributor Author

@jd interesting, that looks close, except for the note in the documentation that says:

This status must not be used in GitHub Branch Protection
as it can block Mergify to merge pull requests.

Are there any status that can be used in the GitHub Branch Protection?

@sileht
Copy link
Member

sileht commented Dec 17, 2018

For the legacy mergify/pr, when you put it in Github Branch Protection, the state can never be success, because Mergify was waiting that mergify/pr goes green to change mergify/pr to success, chicken-egg issue.

#324 or the legacy "mergify/pr" have never worked within the GitHub Branch Protection, it's not 100% reliable because of two things:

  • The asynchronous nature of the Github Branch Protection state mergeable_state attribute of pull request. We seen that computing meageable_state at best takes between 5-10 seconds, but we regularly seen PR when it takes many minutes.
  • And when the PR is blocked we have no way to known why, no detail. Is that mergify/pr that was "pending" and if we retry is 2 minutes it will be unblocked or is it another GitHub Branch Protection settings that blocks the pull request.

Anyways, I clearly get the use-case and the value behind putting Mergify within the Github Branch Protection. I will continue to think about it, I have some remaining ideas to test, to workaround the lake of predictability of the mergeable_state attribute. And I hope to be able to remove this documentation warning.

@edahlseng
Copy link
Contributor Author

Thanks for the explanation @sileht, that helps a lot! Let us know if there's anything we can help with from our end!

@edahlseng
Copy link
Contributor Author

@sileht, wanted to pick this one up again. Is there something that we could try in order to make this happen? If you can point me towards a preferred approach, I can put up a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants