cabal icon indicating copy to clipboard operation
cabal copied to clipboard

implement mergify rules for release branches

Open geekosaur opened this issue 1 year ago • 5 comments

We only handled the case of backports previously, but the current release checklist expects that we can commit PRs to release branches during a release (e.g. changelogs, because we want the list of changelog.d files that are actually part of the release).

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • [x] Patches conform to the coding conventions.
  • [ ] Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions). — It is, but Mergify only uses the config file on master

geekosaur avatar Jun 21 '24 23:06 geekosaur

Naturally, it turns out that Mergify has no way to say "doesn't match regex". I guess there was a reason we didn't have rules for it. (ETA: no, it's just really sensitive to how you write it)

geekosaur avatar Jun 21 '24 23:06 geekosaur

The rules I implemented are a hybrid of the master and backport rules:

  • 2 reviews required
  • no delay between setting the merge label and merging

We should probably iron out what we want the actual rules to be.

geekosaur avatar Jun 22 '24 00:06 geekosaur

And while we're thinking about merge rules, should we prevent merging PRs with a blocked: label?

geekosaur avatar Jun 22 '24 00:06 geekosaur

And while we're thinking about merge rules, should we prevent merging PRs with a blocked: label?

If that's not too fiddly, sounds good.

Mikolaj avatar Jun 22 '24 07:06 Mikolaj

Waiting on #10136

geekosaur avatar Jun 22 '24 23:06 geekosaur

I believe some documentation of our Mergify setup is hosted in CONTRIBUTING.md. Could it be updated to explain, in plain English, how this PR extends the setup? Maybe, at this point, and given recent talks about usefulness of Mergify (see the last meeting notes -- there's a great rundown of the Mergidy features we currently use), Mergify deserves a subsection in that document?.. Maybe a wiki page and a link to it from that doc would be fine, 'cause a casual contributor doesn't have to know all the nits and grits...

ulysses4ever avatar Jul 23 '24 02:07 ulysses4ever

Casual contributors won't be involved with this. It's specifically for commits during releases that are made to the release branch and "backported" to master, such as the changelog updates. Without these rules, they must be committed manually because Mergify will ignore non-backport commits not to master. So if they need to be documented anywhere, it would be the release guide in the wiki.

geekosaur avatar Jul 23 '24 02:07 geekosaur

Casual contributors won't be involved with this.

Yes, that's why I'm suggesting an alternative to shoving all the Mergify-related stuff into CONTRIBUTING.md and to create a wiki page instead. On the other hand, wiki pages end to bit rot...

So if they need to be documented anywhere, it would be the release guide in the wiki.

SGTM!

ulysses4ever avatar Jul 23 '24 02:07 ulysses4ever

Re the meeting notes (https://hackmd.io/36ipih8STyyBV9kvSYi2Dw), I note that GitHub does not support the rules defined in this PR. It defines per-branch rules, so we would have to use one reviewer always on release branches (these rules specify two for non-backports, as with master). There is also no way to specify any delay with GitHub branch protection, from what I saw.

geekosaur avatar Jul 23 '24 03:07 geekosaur

Additionally, Mergify's (but not GitHub's, AFAICT) "squash" method ensures the PR is mentioned in the commit message. @fgaz considered this important for changelog-d to work correctly.

geekosaur avatar Jul 23 '24 03:07 geekosaur

Any other comments here, or should I add the label?

geekosaur avatar Aug 22 '24 00:08 geekosaur

Fire it! :fire: :fire: :fire:

ulysses4ever avatar Aug 22 '24 02:08 ulysses4ever

Hm, I think this will cause problems with #10260, I'm surprised it's not complaining about conflicts.

geekosaur avatar Aug 22 '24 02:08 geekosaur

@mergify refresh

geekosaur avatar Aug 22 '24 02:08 geekosaur

refresh

✅ Pull request refreshed

mergify[bot] avatar Aug 22 '24 02:08 mergify[bot]