The RM should not be expected to read and accept the code itself, but his role should be limited to accepting the idea behind the patch and making sure it is compatible with the rules in place. As such, removing the RM-approval mark is not yielding any benefits.
Moreover, based on the ideas proposed above (any modification removes the reviewed marker), if the scenario depicted by Ralph happens again I would argue that the reviewers would have done a sloppy job at allowing the patch to drift from it's original specification (the one approved by the RM). George. On Thu, Feb 5, 2015 at 10:33 AM, Ralph Castain <r...@open-mpi.org> wrote: > > On Feb 5, 2015, at 7:17 AM, Howard Pritchard <hpprit...@gmail.com> wrote: > > Hi Jeff > > Gilles ideas are great. > > I agree with your RM stamp of approval policy. No removal of rm approved > in the event of subsequent commits. > > > I disagree, so perhaps that should be something settable by the RM for a > given release? I’ve been burned before where I approved something, then > someone added a bunch of unrelated code that caused a significant change > (i.e., modifying prior behavior) without warning. Result: users yelling, > chasing it down, reverting half of the commit, and then re-releasing. > > I’d rather not have that happen again. > > Howard > On Feb 5, 2015 5:04 AM, "Jeff Squyres (jsquyres)" <jsquy...@cisco.com> > wrote: > >> Gilles came up with a cool idea for the OMPIBot (see below). We can do >> this idea, but I want to make sure that everyone is ok with it first. >> >> Consider this scenario: >> >> 1. You create a PR >> 2. Over time, it gets reviewed, and then RM approved (i.e., the >> "reviewed" and "rm-approved" labels are added). >> 3. *** But then new commits are pushed to the PR. >> >> --> Technically, it should really be reviewed again before it is merged. >> Here's what Gilles came up with: >> >> 4. The OMPIBot can tell when new commits are pushed, and can: >> 4a) remove the "reviewed" label, and >> 4b) add the "pushed-back" label >> 5. Further, whenever someone adds the "reviewed" label, OMPIBot can >> automatically remove the "pushed-back" label. >> >> I.e., when you add commits to an already-reviewed PR, you lose >> "reviewed", but you get a positive signal in the form of the "pushed-back" >> label, reminding you that you need to get it reviewed again. And when >> someone reviews it, it automatically removes the "pushed-back" label. >> >> Finally, here's a question to the RM: if someone pushes new commits to a >> PR after it has been rm-approved, do you want the rm-approved label >> removed? My gut feeling is "no" -- it stays approved. >> >> Thoughts? >> >> >> >> On Feb 4, 2015, at 2:26 PM, Howard Pritchard <hpprit...@gmail.com> wrote: >> > >> > +1 >> > great stuff >> > >> > 2015-02-04 5:55 GMT-07:00 Jeff Squyres (jsquyres) <jsquy...@cisco.com>: >> > OMPI devs -- >> > >> > Per lots of previous discussions, you all know that you can't assign >> labels, milestones, or users to issues/pull requests on the ompi-release >> repo. >> > >> > Gilles has written a Github bot that will allow you to do these things >> by inserting special tokens in the text of issues/pull requests/comments. >> Here's an example: >> > >> > This PR fixes problem XYZ. >> > >> > label:bug >> > label:enhancement >> > milestone:v1.8.5 >> > assign:@jsquyres >> > >> > *** PLEASE GO TRY IT on the sandbox ompi-release-bot repo. >> > >> > Here's a fuller explanation of what OMPIBot does, and links to where >> you can try it out: >> > >> > https://github.com/open-mpi/ompi-release-bot/wiki >> > >> > Once we get enough people to try it/fix any bugs/etc., we'll deploy it >> on the ompi-release repo. >> > >> > -- >> > 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/16924.php >> > >> > _______________________________________________ >> > 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/16925.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/16927.php >> > _______________________________________________ > 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/16935.php > > > > _______________________________________________ > 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/16936.php >
