Status checks not re-run when used with merge queue

[iainlane]

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

	// If a PR is added to the merge queue, presumably the policy existed and was valid at the time of merge,
	// so we're just checking for the existance of a policy here and don't care about its validity.
	fetchedConfig := h.ConfigFetcher.ConfigForRepositoryBranch(ctx, client, owner, repository, baseBranch)
	if fetchedConfig.Config == nil {
	return nil
	}
	contextWithBranch := fmt.Sprintf("%s: %s", h.PullOpts.StatusCheckContext, baseBranch)
	state := "success"
	message := fmt.Sprintf("%s previously approved original pull request.", h.AppName)
	status := &github.RepoStatus{
	Context: &contextWithBranch,
	State: &state,
	Description: &message,
	}
	if err := PostStatus(ctx, client, owner, repository, headSHA, status); err != nil {
	logger.Err(errors.WithStack(err)).Msg("Failed to post status check for merge group")
	}

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. 🙂

[bluekeyes]

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.

[iainlane]

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.