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.

Sign up for GitHub

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

Jump to bottom

implement mergify rules for release branches #10135

Merged
merged 4 commits into from
Aug 24, 2024
Merged

implement mergify rules for release branches #10135

merged 4 commits into from
Aug 24, 2024

Conversation

geekosaur
Copy link
Collaborator

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:

  • 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 geekosaur force-pushed the mergify-release-branches branch 3 times, most recently from 9708e9b to 44629e7 Compare June 21, 2024 23:35
@geekosaur
Copy link
Collaborator Author

geekosaur commented Jun 21, 2024 •
edited
Loading

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@Mikolaj
Copy link
Member

Mikolaj commented Jun 22, 2024

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
Mikolaj approved these changes Jun 22, 2024
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense and we can tune it later on if anything proves cumbersome.

@geekosaur
Copy link
Collaborator Author

Waiting on #10136

@geekosaur geekosaur mentioned this pull request Jun 29, 2024
Merged
2 tasks
@ulysses4ever
Copy link
Collaborator

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

@geekosaur
Copy link
Collaborator Author

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.

@ulysses4ever
Copy link
Collaborator

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!

@geekosaur
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

ulysses4ever
ulysses4ever reviewed Aug 8, 2024
View reviewed changes
.github/mergify.yml
Comment on lines +1 to +2
# Note: We do not use the rebase strategy to merge PRs, because that
# loses information needed by changelog-d to associate commits with PRs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's pretty sad: I'm not a fan of merge commits. But I guess, we have no choice (short of dropping changelog-d or teaching it new tricks)...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mergify can use commit templates, so future work might be to define one for this case to ensure the PR is mentioned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, that would be awesome!

ulysses4ever
ulysses4ever approved these changes Aug 8, 2024
View reviewed changes
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Sorry that it took me so long to get here!

@geekosaur
Copy link
Collaborator Author

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

@ulysses4ever
Copy link
Collaborator

Fire it! 🔥 🔥 🔥

@geekosaur geekosaur added the squash+merge me Tell Mergify Bot to squash-merge label Aug 22, 2024
@geekosaur
Copy link
Collaborator Author

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

@geekosaur
Copy link
Collaborator Author

@mergify refresh

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 22, 2024

refresh

✅ Pull request refreshed

@geekosaur
implement mergify rules for release branches
1db7704 
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).
@geekosaur
block merging if PR has a 'blocked:' label
9b8aa7a
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 24, 2024
@mergify mergify bot merged commit dd123e6 into master Aug 24, 2024
51 checks passed
@mergify mergify bot deleted the mergify-release-branches branch August 24, 2024 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulysses4ever ulysses4ever ulysses4ever approved these changes

@Mikolaj Mikolaj Mikolaj approved these changes

Assignees
No one assigned
Labels
continuous-integration merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@geekosaur @Mikolaj @ulysses4ever