googleapis / repo-automation-bots

A collection of bots, based on probot, for performing common maintenance tasks across the open-source repos managed by Google on GitHub.
Apache License 2.0
620 stars 122 forks source link

Saying goodbye to Merge on Green #2871

Open JustinBeckwith opened 2 years ago

JustinBeckwith commented 2 years ago

I think it's time to say goodbye to Merge on Green bot, and I wanted to start a discussion here. GitHub recently introduced the concept of auto-merge which is available on the PR merge button: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request

It largely does the same thing as merge on green: when all approvals are in place, and all required status checks pass, it will automatically merge the PR.

One reason we had held up on pushing further here is that the label was a useful tool for making mass changes. The gh command line tool now provides the ability to request an auto-merge when all CI passes:

gh pr merge --help
Merge a pull request on GitHub.

Without an argument, the pull request that belongs to the current branch
is selected.            

USAGE
  gh pr merge [<number> | <url> | <branch>] [flags]

FLAGS
      --admin            Use administrator privileges to merge a pull request that does not meet requirements
      --auto             Automatically merge only after necessary requirements are met
  -b, --body text        Body text for the merge commit
  -F, --body-file file   Read body text from file (use "-" to read from standard input)
  -d, --delete-branch    Delete the local and remote branch after merge
      --disable-auto     Disable auto-merge for this pull request
  -m, --merge            Merge the commits with the base branch
  -r, --rebase           Rebase the commits onto the base branch
  -s, --squash           Squash the commits into one commit and merge it into the base branch

INHERITED FLAGS
      --help                     Show help for command
  -R, --repo [HOST/]OWNER/REPO   Select another repository using the [HOST/]OWNER/REPO format

LEARN MORE
  Use 'gh <command> <subcommand> --help' for more information about a command.
  Read the manual at https://cli.github.com/manual

What would be stopping us from reducing some footprint here, and using the native GitHub feature?

tmatsuo commented 2 years ago

In general, I agree.

One use case MOG provides is "do not merge" label. I don't think Github's automerge respects the "do not merge" label.

Neenu1995 commented 2 years ago

do-not-merge functionality is very important to us in managing what features and dependencies get merged. Especially when multiple people work on these and use bulk merge options.

I would like to keep the do-not-merge option if possible.

danielbankhead commented 2 years ago

I think the do not merge label would cause a check to fail, thus Github's Auto-merge wouldn't merge it:

tmatsuo commented 2 years ago

@danielbankhead That's true only if the branch protection rule has Do not merge check mandatory.

@Neenu1995 One alternative is to use Draft PR. You need to change the workflow though.

Neenu1995 commented 2 years ago

This happens mostly when a dependency update we are not ready for, is opened by renovate bot.

tmatsuo commented 2 years ago

@Neenu1995

I see. Then our option is either of

  1. Make "Do Not Merge" check mandatory and use Github's automerge
  2. Keep MOG, optionally we can simplify implementation (respect "Do Not Merge" + mergeable property)
Neenu1995 commented 2 years ago

I am happy as long as there is some way to prevent a PR from merging during mass merges.

Option 1 should work fine.

tmatsuo commented 2 years ago

@Neenu1995 Thanks.

For the Option 1, we still need to change the behavior of "Do Not Merge" bot. Current behavior is it only add a check when we add the label.

In order to make the check mandatory, the bot needs to submit a successful status check for all the PRs unless there's the label.

lesv commented 2 years ago

As long as it's as easy. I'm happy. DO-NOT-MERGE should be a required status anyway.

tmatsuo commented 2 years ago

It turned out it's already implemented :)

You need to have .github/do-not-merge.yml with the following content:

alwaysCreateStatusCheck: true

Then you can safely make it a mandatory check.

Neenu1995 commented 2 years ago

@tmatsuo Thank you. I will set up the java libraries next week. What is the timeline for M-O-Gs retirement?

tmatsuo commented 2 years ago

@Neenu1995

No specific date, so no rush. Also you can start using Github's automerge feature without waiting for MOG's retirement :)

lesv commented 2 years ago

Can someone add --auto to repo ??

JustinBeckwith commented 2 years ago

Yeah, we'd likely update the repo merge command to always pass that option, because you don't really want to merge a PR with failing CI if you're an admin :) That was part of the original motivation of merge-on-green!

tbpg commented 2 years ago

I'd consider creating a completely new check with a different name and making that a required check, rather than having the Do Not Merge bot be required. Then, in the future, when there are other reasons to block PRs from submitting, we can use the same (already required!) check.

Of course, that doesn't need to block turning down MoG given the DNM bot can already be configured to always give a status check.

bcoe commented 2 years ago

I'd consider creating a completely new check with a different name and making that a required check

My preference, for now, would be starting simple and making do-not-merge a required check. I don't think this would significantly change anyone's workflow, if you don't want the merge blocked, just remove the label.


I agree that it would be nice to move towards GitHub's auto-merge functionality, the one thing that I'm not 100% sure if we get, is whether it will keep branches updated? We require not just that tests pass, but that a PR is up to date with main.

Depending on how GitHub approaches this, I could imagine us simplifying the approach we take to just ensure branches are up-to-date, if an automerge label has been added.

codyoss commented 2 years ago

As a person that just switched to using the Github feature I have found one big drawback for my usecases. One of my favorite features of this bot is that once you add the label it will keep the branch up-to-date and try to get a change in sometime in the future. The GH feature does not do this. This can make it hard to land changes in mono-repos, or really any high traffic repo, where you require branches be up to date.

tmatsuo commented 2 years ago

@codyoss Thanks for the input. That's an important point.

My suggestion is to only keep that functionality, potentially creating a new bot, something like "Up-to-date bot".

parthea commented 2 years ago

~One thing that we should test when we move away from merge on green is the commit message text. I've noticed that the Merge on green bot takes the PR title and uses that as the commit message, whereas when I use the squash and merge button on Github I notice that the commit message is not the PR title. Instead the commit message from the first commit is used.~ This is no longer an issue now that release please allows you to (easily) fix release notes.