I agree that non-committer review make more sense if the reviewer has established some level of credibility and involvement in the community.
Ultimately, the final committer has to be a community member and they will have final responsibility for the code being Apache compliant and NiFi ready, so even reviews by less well known members of the community should still be low risk. On Fri, Jul 7, 2017 at 9:24 AM, Bryan Bende <bbe...@gmail.com> wrote: > I agree with encouraging reviews from everyone, but I lean towards > "binding" reviews coming from committers. > > If we allow any review to be binding, there could be completely > different levels of review that occur... > > There could be someone who isn't a committer yet, but has been > contributing already and will probably do a very thorough review of > someone's contribution, and there could be someone else who we never > interacted with us before and writes "+1 LGTM" on a PR and we have no > idea if they know what they are talking about or if they even tried > the contribution to see if it works. Obviously a committer can also > write "+1 LGTM", but I think when that comes from a committer it holds > more weight. > > I think we may also want to clarify if we are only talking about > "submitted by committer, reviewed by non-committer" or also talking > about "submitted by non-committer, reviewed by non-committer". > > For the first case I can see the argument that since the contribution > is from a committer who is already trusted, they can make the right > judgement call based on the review. Where as in the second case, just > because a community member submitted something and another community > member says it looks good, doesn't necessarily mean a committer should > come along and automatically merge it in. > > > On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue > <michael.p.hogu...@gmail.com> wrote: > > Thanks for fielding the question, Tony. > > > > Joe and James' statements both make sense. I suppose a case by case > > analysis could be carried out, too. For example, since I'm mostly > > unfamiliar with the code base but am looking to gain familiarity, I'm > > reviewing pretty straightforward or trivial PRs. My plan was to continue > > doing that until I felt comfortable reviewing something with a larger > > impact, such as the new TCPListenRecord processor implementation [1]. > > However, as Tony explained, my original question was whether that sort of > > review would be binding or whether I should be doing it at all. I think > > both of those questions were answered here in that ultimately committer > > sign off is needed, but reviews may be binding regardless of source. > > > > Thanks for the feedback! > > > > Mike > > > > > > [1] https://github.com/apache/nifi/pull/1987 > > On Fri, Jul 7, 2017 at 01:14 James Wing <jvw...@gmail.com> wrote: > > > >> We should definitely encourage review feedback from non-committers. > >> Getting additional perspectives, interest, and enthusiasm from users is > >> critical for any project, doubly so for an integrating application where > >> committers cannot be experts in all the systems we are integrating > with. I > >> believe NiFi could use more review bandwidth. Are missing out on > potential > >> reviewers because of the current policy? > >> > >> I do not have any experience with non-committer "binding reviews" as > >> described in the Apache Gossip thread. How would that work? Wouldn't a > >> committer have to review the review and decide to commit? If we knew > the > >> reviewer well enough to accept their judgement, why not make them a > >> committer? > >> > >> My expectation is that many non-committer reviews are helpful and > >> constructive, but not necessarily 100% comprehensive. Reviewers might > >> comment on the JIRA ticket without working with the PR, or try the > proposed > >> change without reviewing the code, tests, etc. All great stuff, but > >> backstopped by committers. > >> > >> Thanks, > >> > >> James > >> > >> On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <joe.w...@gmail.com> wrote: > >> > >> > It is undefined at this point and I agree we should reach consensus > >> > and document it. > >> > > >> > I am in favor making non-committer reviews binding. > >> > > >> > Why do we do RTC: > >> > - To help bring along new committers/grow the community > >> > - To help promote quality by having peer reviews > >> > > >> > Enabling non-committer reviews to be binding still allows both of > >> > those to be true. > >> > > >> > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <trk...@gmail.com> wrote: > >> > > All, I was having a discussion with Mike Hogue - a recent > contributor - > >> > off > >> > > list, and he had some questions about the review process. And > largely > >> the > >> > > root of the question was, if a non-committer reviews a patch or PR > >> (which > >> > > Mike has spent some time doing), is that considered a "review"? I > >> didn't > >> > > have the answers, so I went on a hunt for documentation. I started > with > >> > the > >> > > Contributor Guide [1]. The guide describes reviewing, and calls out > a > >> > > Reviewer role, but doesn't specifically point out that Reviewer is a > >> > > committer, just that a committer "can actively promote contributions > >> into > >> > > the repository", and goes on to imply the non-committers can review. > >> > > > >> > > Given this, I was unable to answer this question: > >> > > If a committer "X" submits a patch or PR, it is reviewed by a > >> > non-committer > >> > > "Y", does that review satisfy the RTC requirement, and "X" may > merge in > >> > the > >> > > patch? > >> > > > >> > > I found a related discussion on the email list in March [2], which I > >> > think > >> > > implies at least some of the community assumed the canonical review > >> must > >> > be > >> > > by a committer. I also went back a bit to early days [3], where > Benson > >> > > implied a much less "formal" review process. > >> > > > >> > > What I'm hoping for is hopefully come to a consensus of what our > >> project > >> > > expectations are and document in the Contributor Guide. My > expectation > >> > was > >> > > that non-committers could review, similar to what Sean discussed on > >> this > >> > > thread for Apache Gossip (incubating) [4]. However, I don't believe > >> this > >> > is > >> > > the current consensus. > >> > > > >> > > Thoughts? > >> > > > >> > > Tony > >> > > > >> > > 1. > >> > > https://cwiki.apache.org/confluence/display/NIFI/Contributor > >> > +Guide#ContributorGuide-CodeReviewProcess > >> > > 2. > >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb > >> > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q% > >> > 3Dmg%40mail.gmail.com%3E > >> > > 3. > >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb > >> > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@ > >> > mail.gmail.com%3E > >> > > 4. > >> > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de > >> > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp% > 3Dk7wUpoVgQ% > >> > 40mail.gmail.com%3E > >> > > >> >
