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

.github: Add mergify configuration #3026

Merged
merged 4 commits into from
Nov 3, 2022
Merged

.github: Add mergify configuration #3026

merged 4 commits into from
Nov 3, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 14, 2022

Description

This adds a mergify configuration to our repository with two features:

  • Non-draft PRs with conflicts will receive a comment, notifying the author to fix them. See Conflict thomaseizinger/rust-libp2p#31 for an example.
  • Every PR that has the send-it label will be added to a merge-queue. Alternatively, one can also use @Mergifyio queue to achieve the same thing but I found that cumbersome to type 😅

The configuration file specifies the use of squash merges. The commit message is configured to be the same as what Github normally uses. See thomaseizinger@8f68d70 for an example commit created via the merge queue.

One neat feature here is that we can configure the commit message body in the pull request. See thomaseizinger#27 for an example. If the section ## Commit message body is not present, a fallback of an empty string is used. See thomaseizinger@1cf4f5a and thomaseizinger#29.

Mergify will automatically respect all branch protection rules that are in place. Any CI job that is marked as "required" needs to pass before a PR from the merge queue can land in master. We could specify additional conditions here but I don't think we need any.

The intended workflow with this is as follows:

  1. We review and approve PRs as normal.
  2. No magic happens without explicit action.
  3. Once we are happy with a PR to go in, a maintainer needs to either add the send-it label or comment with @Mergifyio queue.
  4. Prior to adding the label, we would edit the PR description and add a ## Commit message body section with the desired message. This could for example include adding Co-authored-by lines or just a longer commit message explaining what the PR does.
  5. Mergify will take over from there, merge latest master in as necessary and eventually merge the PR into master.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger thomaseizinger linked an issue Oct 14, 2022 that may be closed by this pull request
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

This looks very nice Thomas! :)

@mxinden
Copy link
Member

mxinden commented Oct 17, 2022

This will make our lifes so much easier 🎉

The commit message is configured to be the same as what Github normally uses. See thomaseizinger@8f68d70 for an example commit created via the merge queue.

Do I understand correctly that the commit message is then based on the "Commit message body" in the pull request description?

If so, would it make sense to add this to the template?

https://github.com/libp2p/rust-libp2p/blob/master/.github/pull_request_template.md

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me, minus the template changes.

//CC @galargh for all things related to automation (I hope you don't mind)

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Oct 18, 2022

This will make our lifes so much easier 🎉

I hope so 😊

The commit message is configured to be the same as what Github normally uses. See thomaseizinger@8f68d70 for an example commit created via the merge queue.

Do I understand correctly that the commit message is then based on the "Commit message body" in the pull request description?

Correct.The body is. The commit message title will be created from the PR title.

If so, would it make sense to add this to the template?
https://github.com/libp2p/rust-libp2p/blob/master/.github/pull_request_template.md

We can, I am not sure if it is necessary for all PRs. We don't put a message often these days.

But yeah we can add it. I think we should not put a comment there though because I think that will be taken verbatim into the commit message. Didn't verify that.

Happy for it to be part of the template.

@thomaseizinger
Copy link
Contributor Author

Once this is in, I'd like us to start using (and enforcing) conventional commit messages.

That is easy to enforce with mergify and from there we can auto-generate and changelogs and version bumps!

- name: Add to merge queue
conditions:
# All branch protection rules are implicit: https://docs.mergify.com/conditions/#about-branch-protection
- label=send-it
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know how a PR is going to look like when all the checks have already passed but we didn't set this label yet? Will we be able to merge a PR manually in that case and skip the queue? Or does mergify add another required status check of their own to prevent that from happening?

I'm just curious, we can definitely merge and see. But if it is the case that the "normal" merges are still possible, we might want to think more about how we want to educate the users to use the queue instead of merging stuff directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know how a PR is going to look like when all the checks have already passed but we didn't set this label yet? Will we be able to merge a PR manually in that case and skip the queue? Or does mergify add another required status check of their own to prevent that from happening?

Mergify only inherits the GitHub status checks like "Branch must be up to date". You'd be racing an automation if you try to merge it manually and I doubt you'd win :)

If the queue is currently empty, you can definitely merge manually.

I configured the squash commits such that those two scenarios should be indistuingishable.

If we pay for mergify premium, we can configure more queues where some have a higher priority.

I'm just curious, we can definitely merge and see. But if it is the case that the "normal" merges are still possible, we might want to think more about how we want to educate the users to use the queue instead of merging stuff directly.

Only maintainers can merge PRs so I am unsure who you are referring to with "users".

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd be racing an automation if you try to merge it manually and I doubt you'd win :)

I wouldn't be so sure. Web hook deliveries are not instantaneous so there's definitely some headroom. Anyway, I was just curious how/if they addressed that. It doesn't necessarily have to be a problem. I'm definitely OK with checking it out in practice.

Only maintainers can merge PRs so I am unsure who you are referring to with "users".

Yes, I was referring to those with merge privileges. It's all the members of the teams listed in https://github.com/libp2p/github-mgmt/blob/246f37dac14efff89a5815b8de8ed5fbed2f4e14/github/libp2p.yml.

My thinking is that it'd be nice to have some sort of documentation for the maintainers available if we're changing the "PR merge" process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only maintainers can merge PRs so I am unsure who you are referring to with "users".

Yes, I was referring to those with merge privileges. It's all the members of the teams listed in libp2p/github-mgmt@246f37d/github/libp2p.yml.

My thinking is that it'd be nice to have some sort of documentation for the maintainers available if we're changing the "PR merge" process.

What is nice is that it doesn't fully change it but rather complement it. If no other PRs need to be merged and you have one that is up to date, you can just go and merge it, no need to use the queue!

Copy link
Contributor

Choose a reason for hiding this comment

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

But the queue itself is not standard. And because of that I think it requires some explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am gonna open an issue about this. At the moment we don't have any kind of documentation about our development processes, other than what is in the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We kind of have an issue already: #2808

@galargh
Copy link
Contributor

galargh commented Oct 18, 2022

This is quite exciting :) Looking forward to seeing how it works in practice but it seems it has quite the potential.

I'll keep you posted on when we can merge this: #3010 (comment)

Once this is in, I'd like us to start using (and enforcing) conventional commit messages.

That is easy to enforce with mergify and from there we can auto-generate and changelogs and version bumps!

When we get to it, we should probably enforce them with .githooks too so that committers get feedback sooner than after a push to GitHub.

@thomaseizinger
Copy link
Contributor Author

Once this is in, I'd like us to start using (and enforcing) conventional commit messages.
That is easy to enforce with mergify and from there we can auto-generate and changelogs and version bumps!

When we get to it, we should probably enforce them with .githooks too so that committers get feedback sooner than after a push to GitHub.

Because we squash-merge in here, it only applies to the merge commit and not all individual ones. That one we can control with the PR title and the ## Commit message body section in the PR which is cool because it means we don't need to educate contributors much about it. We can just fix the title and commit body before applying the label and happy days :)

@galargh
Copy link
Contributor

galargh commented Nov 2, 2022

@thomaseizinger The mergify app is now installed for this repo 🚀

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

🚀 Thanks @thomaseizinger thanks @galargh !

@thomaseizinger
Copy link
Contributor Author

Let's go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a merge bot for merging PRs
4 participants