On Tue, Jul 11, 2017 at 7:14 PM, Kenneth Knowles <k...@google.com.invalid> wrote: > > On Tue, Jul 11, 2017 at 4:25 PM, Robert Bradshaw < > rober...@google.com.invalid> wrote: > > > On Tue, Jul 11, 2017 at 8:51 AM, Kenneth Knowles <k...@google.com.invalid> > > wrote: > > > I like the idea of controlling squashing or not explicitly in the > > mergebot > > > invocation. I don't think it needs to be made interactive, but just based > > > on preparing the PR appropriately. > > > > > > I propose this for the default `@asfgit merge`: Don't squash, but reject > > > merges that have commits that are obviously intended to be squashed. > > Force > > > the user to resolve that into exactly the final commits (after review is > > > done). The use of this aspect of git probably means the user is advanced > > > and this is not a burden. > > > > Another option would be to automatically squash such commits into > > their predecessors. I think we can train contributors to label fixup > > commits as such without placing the burden on them to learn more git. > > And even if they are advanced git users, it removes an extra > > round-trip between a PR being approved and submitted (as it's good > > form not to rewrite history too much during a review). > > > > The thing is that "fixup! <prior subject line>" indicates that this fixup > should be reordered and applied to the referenced commit. Squashing in > order is not correct. I think the bot reordering to squash is not a good > idea.
I don't see why reordering in this case is a bad thing (if it applies cleanly--one could even automatically check that the patches commute). > So maybe I wasn't clear about the options I want. I want both of: > > (1) The bot merges the commits exactly as they are (for the > git-knowledgable) > (2) The bot squashes all the commits in order (for casual contributors) > > Way simpler than anything interactive and with no reording by the bot. > The rest of my thoughts were just ways to further avoid messing this up. Yeah, these are the most common, and I highly agree should make it hard to accidentally merge fixup commits. I would like to support the (common) case of an advanced user having fixup commits in the review, but being able to merge without waiting for her to manually squash them after the LGTM. > > > Commits that should be rejected: the standard "fixup!" or "squash!" > > > prefixes (probably "fixup" and "squash" without punctuation) and maybe > > any > > > commit message with "review" in it since we get a lot of commits like > > > "respond to review comments". > > > > > > For `@asfgit merge squash` (non-default): still reject "fixup!" and > > > "squash!" commits because those are instructions that might result in > > > reordering and conflicts. Instead, target just those PRs that are a > > > sequence of rambling commits added for review, and always squash into the > > > first. > > > > I think it's fine to squash such commits--when squashed (in order) > > they shouldn't cause any different conflicts than when applied > > separately (and of course any non-clean merges should be rejected > > already). > > > > I'm wary of the bot being too intelligent without validation, or > > accepting a too specialized language of commits (again, without > > validation?). My ideal workflow would likely be something like. > > > > 1) For every approved PR, a "suggested" merge action is presented > > (e.g. in the form of a git rebase -i script) where fixups, if any, are > > automatically squashed. We could refine the logic of when to squash > > over time (perhaps even in response to history). > > 2) A committer could then accept or reject this sequence, kicking of > > the serialized test/merge. > > 2b) A committer could present their own revised rebase -i script > > script instead. Possible "rename" would actually allow an edit of the > > commit title, and no outright omissions allowed. Maybe this is getting > > two complex. (?) > > 2b') A committer could push their own proposed history to the PR, > > using whatever tools they want. (?) > > 3) PR is tested and merged in the background. Happiness abounds. > > > > > On Tue, Jul 11, 2017 at 8:07 AM, Ismaël Mejía <ieme...@gmail.com> wrote: > > > > > >> Thanks a lot Jason, > > >> > > >> Great that Infra solved (2) so fast. > > >> > > >> About (3), maybe the extra pause/validation is not needed, because the > > >> bot will in principle make its work appropriately, maybe what we could > > >> just have is a way to see the git branch with the commits that > > >> mergebot will do as part of the review process similar to the view we > > >> have for the staging website, just as an extra validation but in the > > >> same step. > > >> > > >> About the squashing parameter I think it is a good idea, but I am > > >> afraid that this ends up becoming more complex/interactive because > > >> squashing can imply to rewrite the commit message so I am not sure if > > >> this achievable (or worth the effort). > > >> > > >> WDYT ? > > >> > > >> On Tue, Jul 11, 2017 at 10:37 AM, Aljoscha Krettek <aljos...@apache.org > > > > > >> wrote: > > >> > +1 > > >> > > > >> > This is excellent! > > >> > > > >> >> On 10. Jul 2017, at 21:42, Jason Kuster <jasonkus...@google.com. > > INVALID> > > >> wrote: > > >> >> > > >> >> (quick update re #2 above): ~4 minutes after I reopened the ticket, > > it's > > >> >> fixed. > > >> >> https://github.com/apache/infrastructure-puppet/commit/ > > >> 709944291da5e8aea711cb8578f0594deb45e222 > > >> >> updates the website to the correct address. Infra is once again the > > >> best. > > >> >> > > >> >> On Mon, Jul 10, 2017 at 12:38 PM, Jason Kuster < > > jasonkus...@google.com> > > >> >> wrote: > > >> >> > > >> >>> Glad to hear everyone's pretty happy about it! Have a couple answers > > >> for > > >> >>> your questions. > > >> >>> > > >> >>> Ted: I believe the MFA stuff (two-factor auth on github) is > > necessary > > >> for > > >> >>> getting the additional features on GitHub (reviewer, etc), but may > > not > > >> be > > >> >>> necessary for MergeBot. I'll check in with Infra and get back to > > you. > > >> >>> > > >> >>> Ismaël: Great questions! Answered below. > > >> >>> > > >> >>> 1. The code will likely be transitioned over to an Infra-controlled > > >> >>> repository, but for now is under my account: https://github.com/ > > >> >>> jasonkuster/merge-bot. It's written in Python, so Python aficionados > > >> >>> especially feel free to take a look, kick the tires, and open PRs. > > >> >>> > > >> >>> 2. Glad to hear mergebot worked for you. :) The website not showing > > >> >>> appears to be an issue with transitioning to GitBox; it seems a > > >> reference > > >> >>> may have not been updated. Thanks for the report! I've reopened > > >> >>> https://issues.apache.org/jira/browse/INFRA-14405 to track. > > >> >>> > > >> >>> 3. I'd love to chat about this more! It's totally possible to have > > >> >>> mergebot pause and show the status of the repository before it does > > the > > >> >>> final push, but given that mergebot is merging PRs serially I don't > > >> want to > > >> >>> have someone forget to click "ok" and block other people's PRs. One > > >> other > > >> >>> option would be to allow the person requesting the merge to say > > >> something > > >> >>> like "@asfgit merge squash" or "@asfgit merge nosquash", > > parametrizing > > >> the > > >> >>> merge request. Thoughts? > > >> >>> > > >> >>> On Mon, Jul 10, 2017 at 10:52 AM, Mark Liu > > <mark...@google.com.invalid > > >> > > > >> >>> wrote: > > >> >>> > > >> >>>> +1 Awesome work! > > >> >>>> > > >> >>>> Thank you Jason!!! > > >> >>>> > > >> >>>> Mark > > >> >>>> > > >> >>>> On Mon, Jul 10, 2017 at 10:05 AM, Robert Bradshaw < > > >> >>>> rober...@google.com.invalid> wrote: > > >> >>>> > > >> >>>>> +1, this is great! I'll second Ismaël's list requests, especially > > 1 > > >> and > > >> >>>> 3. > > >> >>>>> > > >> >>>>> On Mon, Jul 10, 2017 at 2:09 AM, Ismaël Mejía <ieme...@gmail.com> > > >> >>>> wrote: > > >> >>>>>> Excellent!, Automation of such repetitive (and error-prone) > > tasks is > > >> >>>>>> strongly welcomed. > > >> >>>>>> > > >> >>>>>> Thanks for making this happen Jason! > > >> >>>>>> > > >> >>>>>> Some comments: > > >> >>>>>> > > >> >>>>>> 1. I suppose the code of mergebot is now part of Apache Infra, > > no? > > >> Do > > >> >>>>>> you know exactly where the code is hosted? And what is the > > procedure > > >> >>>>>> in case somebody wants to improve it or change something in the > > >> >>>>>> future? I suppose other projects can/would benefit of this. > > >> >>>>>> > > >> >>>>>> 2. I configured and used the mergebot with success, however the > > >> >>>>>> website does not reflect the changes of the PR I 'merged', I > > suppose > > >> >>>>>> there are still some things we have to fix, because the changes > > are > > >> >>>>>> not there. > > >> >>>>>> (The PR I am talking about is https://github.com/apache/ > > >> >>>>> beam-site/pull/264) > > >> >>>>>> > > >> >>>>>> 3. Other thing I noticed is that the mergebot didn’t squash the > > >> >>>>>> commits (this probably makes sense) and I didn’t realize this to > > do > > >> it > > >> >>>>>> before because there is not a preview of the state of the actions > > >> that > > >> >>>>>> the mergebot is going to do, can this eventually be improved? (I > > >> don’t > > >> >>>>>> know if this makes sense because this will add an extra > > validation > > >> >>>>>> step and we must trust robots anyway :P). > > >> >>>>>> > > >> >>>>>> This new issue is something that reviewers/committers must > > remember, > > >> >>>>>> and talking about this we need to update this in the contribution > > >> >>>>>> guide to include the configuration/use of the mergebot > > instructions. > > >> >>>>>> > > >> >>>>>> Thanks again Jason and the other who made this possible, this is > > >> >>>> great! > > >> >>>>>> Ismaël > > >> >>>>>> > > >> >>>>>> ps. I’m eager to see this included too for the beam project. > > >> >>>>>> > > >> >>>>>> On Sat, Jul 8, 2017 at 7:28 AM, tarush grover < > > >> >>>> tarushappt...@gmail.com> > > >> >>>>> wrote: > > >> >>>>>>> This is really good!! > > >> >>>>>>> > > >> >>>>>>> Regards, > > >> >>>>>>> Tarush > > >> >>>>>>> > > >> >>>>>>> On Sat, 8 Jul 2017 at 10:20 AM, Jean-Baptiste Onofré < > > >> >>>> j...@nanthrax.net> > > >> >>>>>>> wrote: > > >> >>>>>>> > > >> >>>>>>>> That's awesome ! > > >> >>>>>>>> > > >> >>>>>>>> Thanks Jason ! > > >> >>>>>>>> > > >> >>>>>>>> Regards > > >> >>>>>>>> JB > > >> >>>>>>>> > > >> >>>>>>>> On 07/07/2017 10:21 PM, Jason Kuster wrote: > > >> >>>>>>>>> Hi Beam Community, > > >> >>>>>>>>> > > >> >>>>>>>>> Early on in the project, we had a number of discussions about > > >> >>>>> creating an > > >> >>>>>>>>> automated tool for merging pull requests. I’m happy to > > announce > > >> >>>> that > > >> >>>>>>>> we’ve > > >> >>>>>>>>> developed such a tool and it is ready for experimental usage > > in > > >> >>>> Beam! > > >> >>>>>>>>> > > >> >>>>>>>>> The tool, MergeBot, works in conjunction with ASF’s existing > > >> >>>> GitBox > > >> >>>>> tool, > > >> >>>>>>>>> providing numerous benefits: > > >> >>>>>>>>> * Automating the merge process -- instead of many manual steps > > >> >>>> with > > >> >>>>>>>>> multiple Git remotes, merging is as simple as commenting a > > >> >>>> specific > > >> >>>>>>>> command > > >> >>>>>>>>> in GitHub. > > >> >>>>>>>>> * Automatic verification of each pull request against the > > latest > > >> >>>>> master > > >> >>>>>>>>> code before merge. > > >> >>>>>>>>> * Merge queue enforces an ordering of pull requests, which > > >> ensures > > >> >>>>> that > > >> >>>>>>>>> pull requests that have bad interactions don’t get merged at > > the > > >> >>>> same > > >> >>>>>>>> time. > > >> >>>>>>>>> * GitBox-enabled features such as reviewers, assignees, and > > >> >>>> labels. > > >> >>>>>>>>> * Enabling enhanced use of tools like reviewable.io. > > >> >>>>>>>>> > > >> >>>>>>>>> If you are a committer, the first step is to link your Apache > > and > > >> >>>>> GitHub > > >> >>>>>>>>> accounts at http://gitbox.apache.org/setup. Once the accounts > > >> are > > >> >>>>>>>> linked, > > >> >>>>>>>>> you should have immediate access to new GitHub features like > > >> >>>> labels, > > >> >>>>>>>>> assignees, etc., as well as the ability to merge pull > > requests by > > >> >>>>> simply > > >> >>>>>>>>> commenting “@asfgit merge” on the pull request. MergeBot will > > >> >>>>> communicate > > >> >>>>>>>>> its status back to you via the same mechanism used already by > > >> >>>>> Jenkins. > > >> >>>>>>>>> > > >> >>>>>>>>> This functionally is currently enabled for the “beam-site” > > >> >>>> repository > > >> >>>>>>>> only. > > >> >>>>>>>>> In this phase, we’d like to gather feedback and improve the > > user > > >> >>>>>>>> experience > > >> >>>>>>>>> -- so please comment back early and often. Once we are happy > > with > > >> >>>> the > > >> >>>>>>>>> experience, we’ll deploy it on the main Beam repository, and > > >> >>>>> recommend it > > >> >>>>>>>>> for wider adoption. > > >> >>>>>>>>> > > >> >>>>>>>>> I’d like to give a huge thank you to the Apache Infrastructure > > >> >>>> team, > > >> >>>>>>>>> especially Daniel Pono Takamori, Daniel Gruno, and Chris > > >> >>>>> Thistlethwaite > > >> >>>>>>>> who > > >> >>>>>>>>> were instrumental in bringing this project to fruition. > > >> >>>> Additionally, > > >> >>>>>>>> this > > >> >>>>>>>>> could not have happened without the extensive work Davor put > > in > > >> to > > >> >>>>> keep > > >> >>>>>>>>> things moving along. Thank you Davor. > > >> >>>>>>>>> > > >> >>>>>>>>> Looking forward to hearing your comments and feedback. Thanks. > > >> >>>>>>>>> > > >> >>>>>>>>> Jason > > >> >>>>>>>>> > > >> >>>>>>>> > > >> >>>>>>>> -- > > >> >>>>>>>> Jean-Baptiste Onofré > > >> >>>>>>>> jbono...@apache.org > > >> >>>>>>>> http://blog.nanthrax.net > > >> >>>>>>>> Talend - http://www.talend.com > > >> >>>>>>>> > > >> >>>>> > > >> >>>> > > >> >>> > > >> >>> > > >> >>> > > >> >>> -- > > >> >>> ------- > > >> >>> Jason Kuster > > >> >>> Apache Beam / Google Cloud Dataflow > > >> >>> > > >> >> > > >> >> > > >> >> > > >> >> -- > > >> >> ------- > > >> >> Jason Kuster > > >> >> Apache Beam / Google Cloud Dataflow > > >> > > > >> > >
