Add mergify for PR automation #940

russellb · 2024-04-19T15:04:16Z

commit 6f2ee09
Author: Russell Bryant rbryant@redhat.com
Date: Fri Apr 19 10:59:41 2024 -0400

Add mergify for PR automation

This PR introduces configuration for
Mergify, with an initial configuration
that will automatically merge PRs once they meet a set of defined
criteria.

What I like about this proposed configuration is that it allows the
repo maintainers to manage the explicit merge rules in a file in the
repo itself and then allow a tool to enforce them. The rules
configured in this PR would be:

At least one mainatiner has approved the PR
There are no outstanding review requests
There are no requested changes outstanding
A hold label is not on the PR
The DCO check passes
Other CI jobs (actionlint, lint, test) are required based on which
files were changed in the PR.

Ideally once this configuration is in place, it should be incredibly
rare that anyone hits the "merge" button manually.

There are several other things that can be automated with Mergify -
check out the docs for more if you're interested.

Signed-off-by: Russell Bryant rbryant@redhat.com

russellb · 2024-04-19T15:04:54Z

Submitted as a draft for discussion.

Also, if folks like this, I need to configure the app against the repo and then validate the configuration in Mergify's dashboard before merging this.

russellb · 2024-04-19T16:07:15Z

Here's an emaple PR that was auto-merged in the bot repo: instructlab/instructlab-bot#276

nathan-weinberg · 2024-04-19T17:06:39Z

@russellb IIRC when Mergify was discussed before it was raised that it could take over/replace the labeler GitHub Action functionality we have currently - what are your thoughts on that? Is that something that indeed can/should be done or would this be more something we do on top of that existing automation?

russellb · 2024-04-19T20:10:41Z

@russellb IIRC when Mergify was discussed before it was raised that it could take over/replace the labeler GitHub Action functionality we have currently - what are your thoughts on that? Is that something that indeed can/should be done or would this be more something we do on top of that existing automation?

Yes, it can do auto-labeling! There's no harm in just leaving what you have working already, though. The option to do it in this config would look something like this (not tested, but hopefully you get the idea):

pull_request_rules:
  - name: Auto label documentation PRs that change md files
    conditions:
      - files~=.*\.md$
    actions:
      label:
        toggle:
          - documentation

nathan-weinberg · 2024-04-19T23:56:30Z

Fine either way - happy to swap them later but if it's simple to implement now I'm not opposed to simplifying our infra

russellb · 2024-04-20T00:13:25Z

Fine either way - happy to swap them later but if it's simple to implement now I'm not opposed to simplifying our infra

Having more in the single tool is a bit simpler, to me, but it's only worth it if we're going to use Mergify for more (like auto-merging).

.github/mergify.yml

alimaredia · 2024-04-23T03:32:04Z

@russellb I want to run a scenario by you that has to do with these 3 conditions just to make sure I understand them.

- At least one maintainer has approved the PR
- There are no outstanding review requests
- There are no requested changes outstanding

Say I open up a PR related to a part of the codebase that's Maintainer A's specialty and request their review. Maintainer A requests many changes and I do most of them but leave a few unresolved. Some time after, Maintainer B comes by and approves the PR.

If I resolve the last few requested changes from Maintainer A by hand, and remove Maintainer A's review request would the PR automatically merge (assuming the remaining conditions you've outlined in the draft are fine)?

russellb · 2024-04-23T04:03:48Z

@russellb I want to run a scenario by you that has to do with these 3 conditions just to make sure I understand them.
- At least one maintainer has approved the PR
- There are no outstanding review requests
- There are no requested changes outstanding
Say I open up a PR related to a part of the codebase that's Maintainer A's specialty and request their review. Maintainer A requests many changes and I do most of them but leave a few unresolved. Some time after, Maintainer B comes by and approves the PR.

If I resolve the last few requested changes from Maintainer A by hand, and remove Maintainer A's review request would the PR automatically merge (assuming the remaining conditions you've outlined in the draft are fine)?

I'm not positive on this one. I think we need to test the scenario.

I know that you can drop requests for reviews. If someone has requested changes, I don't think you can do anything to those. They would block until that person comes back and either approves, or at least drops the "request changes" status of their review. We need to test it to validate that's true for sure.

There's also the ability for maintainers to bypass the bot and merge manually if the need arises, like if that maintainer that requested changes went on several weeks of vacation.

This PR introduces configuration for [Mergify](https://docs.mergify.com), with an initial configuration that will automatically merge PRs once they meet a set of defined criteria. What I like about this proposed configuration is that it allows the repo maintainers to manage the explicit merge rules in a file in the repo itself and then allow a tool to enforce them. The rules configured in this PR would be: - At least one mainatiner has approved the PR - There are no outstanding review requests - There are no requested changes outstanding - A `hold` label is not on the PR - The DCO check passes - Other CI jobs (actionlint, lint, test) are required based on which files were changed in the PR. Ideally once this configuration is in place, it should be incredibly rare that anyone hits the "merge" button manually. There are several other things that can be automated with Mergify - check out the docs for more if you're interested. Signed-off-by: Russell Bryant <rbryant@redhat.com>

The `hold` label is already in use, so I didn't remove that one. Both labels can be used to hold a PR for now. Signed-off-by: Russell Bryant <rbryant@redhat.com>

This will remove merged branches after merging them, providing automatic clenaup to forks since merged branches aren't important to keep around. Signed-off-by: Russell Bryant <rbryant@redhat.com>

russellb · 2024-04-23T14:56:09Z

I took it off of draft, as I think this is ready for final review and approval. However, I would prefer it's not actually merged and instead let me work on configuring the application on the repo first before merging the configuration. I can handle the merge when the timing is right.

alimaredia · 2024-04-25T15:22:53Z

.github/mergify.yml

+- name: auto-merge
+  description: automatic merge for main with > 1 approved reviews, all requested reviews have given feedback, not held, and CI is successful
+  conditions:
+    - "#approved-reviews-by>=1"


I think we should set this to 2 to begin with and if we think that's too much friction bump it down to 1. I've been doing that with the PRs I've been reviewing over the last day and I think having that second set of eyes works well.

That's not the org policy: https://github.com/instructlab/dev-docs/blob/main/docs/github-merge-strategy.md#approvals-from-project-maintainers

Need to change the policy first, so I say proceed as-is and if we want to change the policy that needs to be a separate PR to dev-docs

Imo The number of approved-by reviewers for a PR shouldn't be uniform across the entire org, it should be on a repo by repo basis especially since that repo isn't public. If I have to open up a separate PR there I will, but I think that's separate from the 1 vs 2 approved-by reviews decision.

That's still a policy decision you need to propose - we shouldn't diverge this PR based on that. Automation needs to enforce existing policy.

Yes - that was my thinking - enforce current policy, which then allows a PR workflow to propose and discuss changes from there

and adding on -- I would expect maintainers to use their own judgment to decide when they feel additional reviews are needed. It's super easy to do. If you add another person to the list of requested reviewers, the PR can not merge until they all review it.

leseb reviewed Apr 22, 2024

View reviewed changes

.github/mergify.yml Show resolved Hide resolved

.github/mergify.yml Show resolved Hide resolved

.github/mergify.yml Show resolved Hide resolved

russellb added 2 commits April 23, 2024 10:29

mergify: Support do-not-merge label.

940338a

The `hold` label is already in use, so I didn't remove that one. Both labels can be used to hold a PR for now. Signed-off-by: Russell Bryant <rbryant@redhat.com>

russellb force-pushed the mergify branch from 6f2ee09 to 940338a Compare April 23, 2024 14:29

mergify: Add delete_head_branch after merge

b571ce8

This will remove merged branches after merging them, providing automatic clenaup to forks since merged branches aren't important to keep around. Signed-off-by: Russell Bryant <rbryant@redhat.com>

russellb marked this pull request as ready for review April 23, 2024 14:55

russellb added the hold In-progress PR. Tag should be removed before merge. label Apr 23, 2024

alimaredia reviewed Apr 25, 2024

View reviewed changes

russellb merged commit 68ebbee into instructlab:main Apr 26, 2024

Add mergify for PR automation #940

Add mergify for PR automation #940

Uh oh!

Conversation

russellb commented Apr 19, 2024

Uh oh!

russellb commented Apr 19, 2024

Uh oh!

russellb commented Apr 19, 2024

Uh oh!

nathan-weinberg commented Apr 19, 2024

Uh oh!

russellb commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathan-weinberg commented Apr 19, 2024

Uh oh!

russellb commented Apr 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alimaredia commented Apr 23, 2024

Uh oh!

russellb commented Apr 23, 2024

Uh oh!

russellb commented Apr 23, 2024

Uh oh!

alimaredia Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

nathan-weinberg Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

nathan-weinberg Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

alimaredia Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

nathan-weinberg Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

russellb Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

russellb Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

russellb commented Apr 19, 2024 •

edited

Loading