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

Add mergify merge queue configuration file #4917

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Nov 9, 2023

Issue Addressed

With bors decommissioned, we need an alternative tool to handle our pull request merging.

The key features that we need are:

  1. Making sure that all checks are passing before merging to our protected branch unstable.
  2. Ability to batch multiple pull requests to save CI and developer time.
  3. Efficiency - only run CI once before merging.
  4. Simplicity, ideally similar experience to bors to ensure a smooth transition.

Proposed Changes

This PR proposes using Mergify merge queue as a replacement and adds a mergify.yml configuration file with some queue rules similar to our existing bors rules as a starting point.

I've also stolen @michaelsproul's changes on success checks from #4254, so that changes in CI jobs don't require modification to mergify configuration on the stable branch.

In addition to this PR, we'd also need:

  • Install mergify on the repository (@paulhauner @AgeManning)
  • This PR merged to stable (mergify only reads the config file from stable branch)
  • Optional: Set up branch protection rule to enforce exclusive Mergify merges (docs)

Once the above in done, we would be able to add PRs to the merge queue by this command: @mergify queue (instead of bors+) - note that this need to be in it's own comment, unlike bors r+ where it could be embeded in a comment.

The default setting waits for 30s before starting a batch (if batch isn't filled), and we could configure this if needed.

Additional Info

I have done some testing on my fork and have verified the following rules:

  • Test base=unstable queue condition (we could remove the target-branch-check)
  • Test label=ready-for-bors queue condition
  • Test #approved-reviews-by >= 1 queue condition
  • Test code conflict with base branch should reject merge
  • Test check-success queue conditions (except for license/cla as I don't have it set up)
  • Test check-success merge conditions
  • Test merge queue batching

Questions

  • Do we use a label like ready-for-bors after approving PR, perhaps ready-for-merge?
    • UPDATE: yes, label is renamed to ready-for-merge

.github/mergify.yml Outdated Show resolved Hide resolved
@jimmygchen jimmygchen changed the base branch from stable to unstable November 28, 2023 06:08
@jimmygchen jimmygchen force-pushed the mergify-conf branch 2 times, most recently from 59a57a7 to 9d9d82b Compare November 28, 2023 06:10
…bs don't require modification to mergify configuration on stable branch.
@jimmygchen jimmygchen added ready-for-review The code is ready for review v4.6.0 ETA Q1 2024 and removed do-not-merge RFC Request for comment labels Nov 28, 2023
@paulhauner
Copy link
Member

Install mergify on the repository (@paulhauner @AgeManning)

Done!

Optional: Set up branch protection rule to enforce exclusive Mergify merges (docs)

Happy to do this after Mergify is working (I assume we need to do manual merges to get it working in the first place).

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I cross-checked the config against https://docs.mergify.com/configuration/file-format/#queue-rules

I was a bit confused that some of the options there don't seem to have defaults. Some things I noticed:

  • commit_message_template doesn't have a default but I think this is probably fine, so long as it's something like {PR_TITLE} ({PR_NUMBER}).
  • allow_inplace_checks is set to true by default but seems kind of unnecessary as it treats single-PR batches differently from others. I would kind of prefer consistency, and immutability of the original branch (i.e. allow_inplace_checks: false).
  • branch_protection_injection_mode doesn't have a default listed and I'm not sure which setting is better. Probably merge so that the checks only apply when merging and not while queuing? Maybe we can leave it unset as we have queue_conditions instead? And/or we don't have branch protection rules?

.github/mergify.yml Show resolved Hide resolved
@jimmygchen
Copy link
Member Author

@michaelsproul thanks for the review!

  • commit_message_template: yeah from memory the default is something similar to what we have, but I'll re-test again and report back!
  • allow_inplace_checks: this is an optimization but some users have reported that mergify only runs on up to date PRs if this is set to false, which is a bit annoying. Most of the defaults seem sensible though, I'll do some more testing on outdated branches.
  • branch_protection_injection_mode: not sure why the doc doesn't state a default there, but I'm pretty sure the default respects the branch protection rules. If I remember correctly the default is queue - so it wouldn't queue the PR without an approval if the repo has a min 1 approval setting on the branch.

@jimmygchen
Copy link
Member Author

I've done another round of testing on my fork:

commit_message_template

the default commit message is the same as if it was merged using the GitHub UI.
image

allow_inplace_checks

I didn't modify this, but I tried queueing a PR that's not up to date with unstable, and mergify ended up creating a PR with the update commit: jimmygchen#45

So I'm happy to leave these settings as it is.

@michaelsproul
Copy link
Member

Nice, looks good!

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Dec 12, 2023
@jimmygchen jimmygchen merged commit 4cf4819 into sigp:unstable Dec 12, 2023
27 checks passed
@jimmygchen
Copy link
Member Author

Let's merge! 🎉

@jimmygchen jimmygchen linked an issue Dec 12, 2023 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra-ci ready-for-merge This PR is ready to merge. v4.6.0 ETA Q1 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move away from deprecated bors-ng
3 participants