My personal opinion on this is that adding a bot:rebase command is a bit silly. IMO only the author of the PR should be allowed to issue this command, since it modifies his/her fork repo, in which case why not just use the git command line to do this? We shouldn't be implementing a full copy of the git CLI via GH issue comments.
The bot label/milestone commands are mainly workarounds for GitHub permission deficiencies. We wouldn't need them if we could properly delegate permissions at a finer granularity. -Dave On Feb 5, 2015, at 3:31 PM, Jeff Squyres (jsquyres) <jsquy...@cisco.com> wrote: > Ralph and I chatted more about this on the phone. > > Short version: we think we generally agree. :-) > > A point that was missed in the prior email discussion was that when we click > the green "merge" button, it puts effectively those commits at the HEAD -- > which, for the purposes of this conversation, is close-enough to rebasing > such that rebasing and re-smoke-testing is not a bad thing. > > Is this a bot you guys can write? I.e., I think it should probably be > different than the label/milestone/assignment bot. > > > > >> On Feb 5, 2015, at 2:58 PM, Mike Dubman <mi...@dev.mellanox.co.il> wrote: >> >> rebase before merge is a good practice/gate used by other code review tools >> (like gerrit). >> >> it helps to keep git history linear (less merge commits) and takes the most >> recent patch set from PR and have it rebased onto the tip of the destination >> branch. If rebase succeeds (no conflicts) - jenkins will smoke-test it and >> RM will feel more confident that rebased PR is up to date with smoke testing >> and operational/compilable state. >> >> smoketest/jenkins is not competing with mtt or other forms of testing >> anyway, just brutal indication of project health. :) >> >> >> >> >> On Thu, Feb 5, 2015 at 9:17 PM, Jeff Squyres (jsquyres) <jsquy...@cisco.com> >> wrote: >> Thinking about this a little bit, there's a wrinkle: you (the individual >> developer) will need to give push permissions on your ompi / ompi-release >> fork to the OMPIBot Github account. Otherwise, it won't be able to push >> back to your fork. >> >> Thinking about this even more, I'm a little worried about implementing this >> feature. It seems to give a lot of credence to the smoke test -- i.e., if >> hello world/ring work, then my patch must work. I'm not sure that's >> "enough" to give me confidence that a patch rebased properly. >> >> Thoughts? >> >> >>> On Feb 5, 2015, at 2:08 PM, Jeff Squyres (jsquyres) <jsquy...@cisco.com> >>> wrote: >>> >>> Mike: >>> >>> This sounds good, but let us get the label/milestone/assign thing going >>> first. >>> >>> I'm thinking that the functionality you describe may become a different >>> bot...? I'm not sure. >>> >>> >>>> On Feb 5, 2015, at 9:56 AM, Mike Dubman <mi...@dev.mellanox.co.il> wrote: >>>> >>>> yep, exactly. >>>> >>>> >>>> On Thu, Feb 5, 2015 at 2:35 PM, Jeff Squyres (jsquyres) >>>> <jsquy...@cisco.com> wrote: >>>> On Feb 5, 2015, at 7:20 AM, Mike Dubman <mi...@dev.mellanox.co.il> wrote: >>>>> >>>>> sounds cool and useful. >>>> >>>> K, thanks. >>>> >>>>> Also, does it make sense to have "rebase" knob to cause "try rebase if no >>>>> conflicts" with upstream? >>>> >>>> Just to be sure what you mean: something like "rebase:" that will cause >>>> the patch set to be rebased to head of master (if there are no conflicts)? >>>> >>>> I think you're asking because: >>>> >>>> - it doesn't make the RM/GK's job easier because github would have already >>>> detected this and still kept the "merge" button green on the PR >>>> - but it would (assumedly) trigger a new Jenkins smoke test, which is the >>>> desirable thing here (i.e., it may merge, but it may or may not *work) >>>> >>>> Is that what you're thinking? >>>> >>>> -- >>>> Jeff Squyres >>>> jsquy...@cisco.com >>>> For corporate legal information go to: >>>> http://www.cisco.com/web/about/doing_business/legal/cri/ >>>> >>>> _______________________________________________ >>>> devel mailing list >>>> de...@open-mpi.org >>>> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >>>> Link to this post: >>>> http://www.open-mpi.org/community/lists/devel/2015/02/16929.php >>>> >>>> >>>> >>>> -- >>>> >>>> Kind Regards, >>>> >>>> M. >>>> _______________________________________________ >>>> devel mailing list >>>> de...@open-mpi.org >>>> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >>>> Link to this post: >>>> http://www.open-mpi.org/community/lists/devel/2015/02/16934.php >>> >>> >>> -- >>> Jeff Squyres >>> jsquy...@cisco.com >>> For corporate legal information go to: >>> http://www.cisco.com/web/about/doing_business/legal/cri/ >>> >>> _______________________________________________ >>> devel mailing list >>> de...@open-mpi.org >>> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >>> Link to this post: >>> http://www.open-mpi.org/community/lists/devel/2015/02/16941.php >> >> >> -- >> Jeff Squyres >> jsquy...@cisco.com >> For corporate legal information go to: >> http://www.cisco.com/web/about/doing_business/legal/cri/ >> >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >> Link to this post: >> http://www.open-mpi.org/community/lists/devel/2015/02/16943.php >> >> >> >> -- >> >> Kind Regards, >> >> M. >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >> Link to this post: >> http://www.open-mpi.org/community/lists/devel/2015/02/16944.php > > > -- > Jeff Squyres > jsquy...@cisco.com > For corporate legal information go to: > http://www.cisco.com/web/about/doing_business/legal/cri/ > > _______________________________________________ > devel mailing list > de...@open-mpi.org > Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel > Link to this post: > http://www.open-mpi.org/community/lists/devel/2015/02/16945.php
