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

Automatically merge with passing tests and a +1 Review #29

Closed
dstufft opened this Issue Feb 15, 2017 · 19 comments

Comments

Projects
None yet
@dstufft
Member

dstufft commented Feb 15, 2017

It would be great if a bot could automatically merge PRs that meet all of the following:

  • All status checks are green
  • A CLA check is good.
  • Someone with write permission to the repository has given it an approved review with no reviews from someone with write permission requesting changes.

Right now if I review someone's PR either one of two things happen:

  • The status checks are green and the CLA check is green, in which case I press the merge button.
    • This case is trivial though it does require an extra button click that could be eliminated.
  • The status checks are pending or the CLA check hasn't happened yet.
    • This is the case I'm most interested in, what happens here is at some point in the future the status checks will go green but there is no notification that this has happened, so this relies on someone coming around again and notice that the merge button can be pushed.
@ronaldoussoren

This comment has been minimized.

ronaldoussoren commented Feb 15, 2017

@brettcannon

This comment has been minimized.

Member

brettcannon commented Feb 15, 2017

As of right now any one committer is sufficient to clear a PR. If we really wanted to we could have a bot that adds a status check for review approval for modules that have a specific owner (although I personally don't like our personal ownership of modules concept so I won't be putting in the effort to construct such a bot).

@zware

This comment has been minimized.

Member

zware commented Feb 15, 2017

We could have a convention of "if the touched module is owned by somebody who is particular about it, leave your 'LGTM' comment but don't click the 'approve' button". I don't think a bot for it would be worth the effort.

@zware

This comment has been minimized.

Member

zware commented Feb 15, 2017

Clarification: I don't think an ownership bot is worth it. I'm all for the auto-merge bot @dstufft proposed.

@dstufft

This comment has been minimized.

Member

dstufft commented Feb 15, 2017

@ronaldoussoren If that's the case, it might be a good idea to add a different feature that automatically added those explicit owners as "request a review" for those modules so they are notified there is a PR that touches the module they "own".

I thought at one point I saw a statement that the required reviews thing would mandate a +1 review from anyone that was explicitly marked as "please make a review" but I can't seem to find that now, so it would need tested to see if that would actually cause the PR to be blocked on waiting on a review from them or not.

@dstufft

This comment has been minimized.

Member

dstufft commented Feb 15, 2017

I mentioned in #32 about the mention bot that could help notify people about things that need reviewed without having to subscribe to the firehouse, but that same bot could also be used for people who own a particular module to always get notified. It also has a mode that will have the bot do an explicit "request a review" from people, but it looks like that's a global only flag, but we could maybe modify the bot so that module owners can flag themselves to have a review requested of them instead of just being mentioned.

@dhimmel

This comment has been minimized.

dhimmel commented Feb 16, 2017

Merging methods: squash, rebase, merge

One thing that would be difficult for a bot is how to merge.

For many pull requests, a GitHub squash merge, where all commits are squashed into one with potential manual creation of a consolidated commit message, is the cleanest. For example, python/cpython#97 is a good example of a squashed pull request.

In other cases, it makes sense to use GitHub's merge (or rebase) option. In these cases, the reviewers may direct the pull request contributor to rewrite (and force push) the commit history so it's clean. This is especially important for pull requests with commits from multiple authors. Squashing the entire PR would erase all but one authors' recognition from the commit history. Additionally, there are some larger PRs that are more readable as a series of commits.

@dstufft

This comment has been minimized.

Member

dstufft commented Feb 16, 2017

We only allow squash merges on CPython's repository.

@dhimmel

This comment has been minimized.

dhimmel commented Feb 16, 2017

We only allow squash merges on CPython's repository.

@dstufft that's makes things easier.

In addition, I could imagine a skip-automerge tag or keyword you could add to an issue, that would disable the bot. That would take care of lot's of these difficult & infrequent cases.

@berkerpeksag

This comment has been minimized.

Member

berkerpeksag commented Feb 19, 2017

I think time zone differences needs to be considered when a PR is opened by another core developer. If someone approves my PR in 3 am in European time zone and if it breaks some non-Linux buildbots then I wouldn't be able to fix the problem quickly.

@brettcannon brettcannon added the bot label Apr 10, 2017

@ncoghlan

This comment has been minimized.

Contributor

ncoghlan commented Jun 19, 2017

@Mariatta pointed out on core-workflow that to do this we'd need some way for a core dev to provide a pre-approved commit message that:

  • amends the syntax of the PR reference in the commit title
  • provides the actual final commit message, not the concatenation of all the individual commit messages along the way
@Mariatta

This comment has been minimized.

Member

Mariatta commented Sep 27, 2017

Our newest bot, @miss-islington, can notify the core dev when status checks on a backport PR is completed (she will leave a comment).
So I think there may be ways to get this to work on regular PRs, i.e. leaving a comment that all status checks are complete, so the core dev can come back and merge the PR.
And the comment should only be left when there is awaiting merge label. (meaning a core dev has approved the PR).

@brettcannon

This comment has been minimized.

Member

brettcannon commented Sep 28, 2017

And based on how the "should core devs merge on other's behalf", probably a label or comment that says "I have signed off on this PR and have no plans to change it later".

@terryjreedy

This comment has been minimized.

Member

terryjreedy commented Sep 28, 2017

I think automerge , especially to master, is a very bad idea unless it is OPT-IN via an automerge tag. Almost all PRs need a human re-written commit message. For many issues, unittests are inadequate, and Travis does not even run them all. I would never use the tag on initial PRs for IDLE. If 'approve' meant 'merge it now', I would never approve any PR.

I might use the tag for backports where I think the supplementary human testing done before merging to master is sufficient to cover 3.6 also.

@Mariatta

This comment has been minimized.

Member

Mariatta commented Sep 28, 2017

I'm also against automerge. So I suggest only leaving a comment, and a core dev can come back and take one last look before doing the merge.

@ncoghlan

This comment has been minimized.

Contributor

ncoghlan commented Sep 29, 2017

I like @Mariatta's suggested approach, as @miss-islington's notifications that CI has completed successfully end up in my main "things on GitHub that I might care to do something about" email folder, whereas if I've approved a PR that's still running CI, I may forget to go back later and actually merge it.

@warsaw

This comment has been minimized.

Member

warsaw commented Sep 29, 2017

I'll snarkily comment that GitLab already has this feature built into its UI, and yes it's amazing for productivity.

@jd

This comment has been minimized.

jd commented Jul 11, 2018

For GitHub, there's Mergify offering this feature as well. It allows to implement exactly what's described in the first post of this issue, for what it's worth (and has some others nifty features).

@Mariatta Mariatta removed the hacktoberfest label Jul 11, 2018

@Mariatta Mariatta referenced this issue Sep 11, 2018

Merged

Automerge PR labeled with "automerge". #146

1 of 1 task complete
@Mariatta

This comment has been minimized.

Member

Mariatta commented Sep 11, 2018

PR for Python's automerge bot: python/miss-islington#146

Mariatta added a commit to python/miss-islington that referenced this issue Sep 11, 2018

Automerge PR labeled with "automerge". (#146)
Merge the PR if:
- PR has "awaiting merge" and "automerge" labels, and
- all CI status checks have passed.

It will:
- replace `#` with `GH-`
- get the commit message from PR title and description

Closes python/bedevere#14
Closes python/core-workflow#29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment