Fix prematurely merging PR after update

[f1sherman]

After my changes in #235, I noticed that sometimes a PR is updated and then merged as part of the same webhook request, before the required statuses are fulfilled. This is because bulldozer was still using the stale statuses from before the update was performed.

This change skips the merge if the PR was updated to avoid using the stale PR status.

[f1sherman]

@bluekeyes sorry for the bother, I'd love your thoughts on this when you have a chance. It's a pretty big deal for us to make sure that all checks are passing after integrating the latest version of the main branch before merging and I'm hoping to get this bug fix into our environment ASAP.

[bluekeyes]

I think this change makes sense, but I'd like to understand more about the situation where it happens. Specifically, I want to understand if this is a normal thing that happens in certain conditions or if Bulldozer is racing against GitHub and catching an open merge window that we expected GitHub to prevent. I think an example of your branch protection settings, Bulldozer settings, and sequence of events that led to one of these merges would help.

For instance, do you have branch protection set to enforce that branches must be up-to-date before merging?

[f1sherman]

Sounds good!

For branch protection settings, the people that can merge to main are limited to a specific group of people. PRs are not required to be up to date with main in branch protection settings but I don't think that would matter in this case.

Bulldozer configuration is set to both update and merge when the ready-for-merge label is added, and requires specific CI statuses before merging.

What I've seen happen is that, given a PR that is behind main and all CI is passing, when the ready-for-merge label is added the branch is updated and merged as part of the same webhook handler instance. This is before CI is run on the updated branch. The reason for this is that, when evaluating whether it should merge, Bulldozer uses the CI statuses from before it performed the update.

This happens in the following code:

bulldozer/server/handler/pull_request.go

Lines 72 to 81 in 19a4b08

	if event.GetAction() == "labeled" || event.GetAction() == "opened" {
	base, _ := pullCtx.Branches()
	if err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, base); err != nil {
	logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request")
	}
	}
	if err := h.ProcessPullRequest(ctx, pullCtx, client, config, pr); err != nil {
	logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request")
	}

You can see how after UpdatePullRequest is called, Bulldozer is still using the (now stale) Github Context for evaluating whether it should merge when ProcessPullRequest is called.

Does that make sense?

[bluekeyes]

Thanks, this all makes sense to me. The situation you described is basically the only one I could think of, so it's good to have confirmation that this is what's happening.

If PRs are required to be up-to-date in branch protection, then I expect GitHub to return an error when Bulldozer attempts to merge the just-updated PR. Bulldozer detects and ignores this error and waits for future events, which would most likely be for the status checks on the update commit.

[f1sherman]

Thank you!

If PRs are required to be up-to-date in branch protection, then I expect GitHub to return an error when Bulldozer attempts to merge the just-updated PR.

That's interesting. I was actually expecting GitHub to allow the merge, since Bulldozer had (just) previously updated it.