palantir
Status checks not re-run when used with merge queue
Policy Bot currently simply posts success if it's run in a merge queue and there's a config in place.
https://github.com/palantir/policy-bot/blob/79ce388e099764a523864d00c53b06a602dc7331/server/handler/merge_group.go#L61-L79
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. 🙂
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.
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.
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:
- For properties that depend on a pull request data, find the original pull request and evaluate using that state
- For (some) properties that depend on commit data, use the commit from the merge queue branch. This wouldn't be all commit data - things like contributor and author conditions should still be evaluated in the context of the PR.
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.
Is this only an issue for policies that use status predicates/conditions? Or does it apply in other situations as well?
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...
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.
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 merge_group event. Or do that, but only if the merge group has more than just the one PR in it. Or something! 😁
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.
I believe policy-bot should definitely evaluate at least parts of the policy again for the merge-group. In our setup (which uses github enterprise), the only required check is policy-bot, and we delegate all the enforcement of workflow runs to policy-bot. So it's the single pace where we enforce required checks, conditionally. Because it doesn't re-evaluate the workflow approval rules (and disapproval rules) for the merge_group, we cannot use merge_queues with policy-bot.
IMHO approval rules that only contain requires rules for approvals, changed files, modified lines etc, it's fine to not reevaluate those approval rules.
However if an approval rule contains conditions related to checks (has_successful_status, has_status, has_workflow_result), then it's absolutely no question that these approval rules must be reevaluated just as if it was a pull request, but they must be evaluated against the merge_group ref instead of individual pull requests.
BTW this comment is flat out wrong in it's assumption because of the circumstances stated above: https://github.com/palantir/policy-bot/blob/33738a0c63e643663d670e186e8ecbc297e00950/server/handler/merge_group.go#L61-L62
We DO want policy-bot to re-evaluate a well defined subset of the approval rules related to status checks and workflow runs applied to the merge group.
Another thing that the policy should reflect though is if the status-check fails a disapproval-rule should also reflect failure so the merge-group can be destroyed instead of just handing.
If the idea is to re-evaluate only a subset of approval rules on the merge group, is it possible that some of those approval rules are themselves dependent on the evaluation of PR-scoped conditions, such as modified lines, etc?
