-
Notifications
You must be signed in to change notification settings - Fork 108
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
Status checks not re-run when used with merge queue #800
Comments
I don't have any personal experience with merge queues yet (they only arrived in GitHub Enterprise in 3.12) so the current behavior is based on discussions with some other users in #548. For that use case at that time, always providing a passing status seemed appropriate. I'm not surprised if there are situations where this doesn't really work.
Is this only an issue for policies that use status predicates/conditions? Or does it apply in other situations as well? Given my understanding of merge queues, I think the challenge for Policy Bot is that the evaluation in a merge queue happens on a branch that is semi-disconnected from a pull request. This makes it hard to evaluate many of the rules that rely on PR state. If you need to re-evaluate policies as part of the merge queue, it sounds like we need some kind of split logic:
I think this is possible, but will probably complicate the evaluation logic. We may also need to consider what happens if a policy change is ahead of a PR in the merge queue. It's possible that when a PR was added to the queue it passed the current policy but policy change ahead of it in the queue changes the approval condition such that it is no longer approved. We'll also probably need to figure out what to do when we receive a status event from GitHub too, since right now we try to look up an open PR from the commit hash. I guess that will also have to expand to consider merge queue branches. |
Thanks for the reply. I'm just coming back to this quickly, mainly to say that while I can see us using & needing this support eventually, it's not likely to be a priority in the short term. So if anyone else is reading who would like to improve merge queue support more quickly, please don't feel like you'd be treading on anyone's toes.
I'm thinking about statuses / workflows mainly, that's correct. It seems like it'd not be necessary to re-check most, if not all, of the others. But this observation you made is very interesting...
That, to me, means that for the evaluation to be 100% correct we'd have to reconsider the entire policy, at least in the situation where the policy changed since the PR was queued up. Perhaps the simplest thing to do would be to read the policy from the merge queue branch instead of the base branch when handling a You could go even deeper, of course, and ask what should happen if the PR's approval/disapproval state changes while it's in the merge queue but not merged yet. To me it would be OK to take small steps rather than trying to tie all of this down at once. |
Policy Bot currently simply posts success if it's run in a merge queue and there's a config in place.
policy-bot/server/handler/merge_group.go
Lines 61 to 79 in 79ce388
This makes a certain amount of sense in its terms - to be able to merge the check must have been green, so why not simply pass once more?
However, this means that checks can't be re-run once a branch is queued for merging, which means that a big advantage of using merge queues - testing the branch again in the state that it actually will be merged - can't be taken advantage of.
We have a fast-moving monorepo and we often see "logical" conflicts, where the state of the base is incompatible with the PR but GitHub is still happy to merge. We can't use anything like "require branches to be up-to-date" because the volume of commits is faster than CI so developers would have a tough time getting anything merged. Merge queues are perfect for this, if we can check the commit is still good once we come to merge it.
Thought it'd be worth a discussion about whether / how Policy Bot could support merge queues a bit more. If it all seems doable then I'm sure I could have a go at it. 🙂
The text was updated successfully, but these errors were encountered: