GeezerVison is showing github.com — Automatically merge with passing tests and a +1 Review · Issue #29 · python/core-workflow
Automatically merge with passing tests and a +1 Review #29DescriptionIt 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.
Reacted by Brett Cannon, Alyssa Coghlan, Dale O'Brien, Guy Romm, Anders D. Johnson, Arnav Borborah, ddlatham and SF-Zhou Activityronaldoussoren commented on Feb 15, 2017 On 15 Feb 2017, at 01:46, Donald Stufft ***@***.***> wrote:
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.
Is having one commiter with an approved review sufficient? There are a number of modules with explicit owners (decimal and itertools spring to mind), for those modules a review of other committers is useful but not necessarily sufficient.
Ronald Reacted by Daniel Himmelstein, Ezio Melotti and Terry Jan Reedy brettcannon commented on 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 commented on 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 commented on 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 commented on 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 commented on 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 commented on 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 commented on Feb 16, 2017 We only allow squash merges on CPython's repository. dhimmel commented on 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 commented on 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. ncoghlan commented on 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 commented on 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 commented on 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 commented on 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 commented on 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 commented on 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 commented on Sep 29, 2017 I'll snarkily comment that GitLab already has this feature built into its UI, and yes it's amazing for productivity. Reacted by Anders D. Johnson, Peter Hausamann, Brenda Wallace, Stefan Profanter and Matt Reacted by Daniel Himmelstein jd commented on 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). Reacted by Brett Cannon, Shumin Liu, Mehdi ABAAKOUK and Julien Danjou Reacted by Julien Danjou Reacted by Julien Danjou Reacted by Julien Danjou Mariatta commented on Sep 11, 2018
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|