If I understood correctly what is proposed is: - Committers to be able to have their PRs reviewed by non-committers and be able to self-merge. - For non-committers nothing changes.
This enables a committer (wearing contributor head) to merge their own changes without committer approval, so we should ensure that no shortcuts are taken just to get things done quickly. I think as Thomas Weise mentioned that mentoring and encouraging more contributors to become committers is a better long term solution to this issue. On Thu, May 31, 2018 at 11:24 PM Eugene Kirpichov <kirpic...@google.com> wrote: > > Agreed with all said above - as I understand it, we have consensus on the > following: > > Whether you're a committer or not: > - Find somebody who's familiar with the code and ask them to review. Use your > best judgment in whose review would give you good confidence that your code > is actually good. > (it's a well-known problem that for new contributors it's often difficult to > choose a proper reviewer - we should discuss that separately) > > If you're a committer: > - Once the reviewers are happy, you can merge the change yourself. > > If you're not a committer: > - Once the reviewers are happy: if one of them is a commiter, you're done; > otherwise, involve a committer. They may give comments, including possibly > very substantial comments. > - To minimize the chance of the latter: if your change is potentially > controversial, involve a committer early on, or involve the dev@ mailing > list, write a design doc etc. > > On Thu, May 31, 2018 at 2:16 PM Kenneth Knowles <k...@google.com> wrote: >> >> Seems like enough consensus, and that this is a policy thing that should >> have an official vote. >> >> On Thu, May 31, 2018 at 12:01 PM Robert Bradshaw <rober...@google.com> wrote: >>> >>> +1, this is what I was going to propose. >>> >>> Code review serves two related, but distinct purposes. The first is just >>> getting a second set of eyes on the code to improve quality (call this the >>> LGTM). This can be done by anyone. The second is vetting whether this >>> contribution, in its current form, should be included in beam (call this >>> the approval), and is clearly in the purview, almost by definition, of the >>> committers. Often the same person can do both, but that's not required >>> (e.g. this is how reviews are handled internally at Google). >>> >>> I think we should trust committers to be able to give (or if a large >>> change, seek, perhaps on the list, as we do now) approval for their own >>> change. (Eventually we could delegate different approvers for different >>> subcomponents, rather than have every committer own everything.) >>> Regardless, we still want LGTMs for all changes. It can also make sense for >>> a non-committer to give an LGTM on another non-committers's code, and an >>> approver to take this into account, to whatever level at their discretion, >>> when approving code. Much of Go was developed this way out of necessity. >>> >>> I also want to point out that having non-committers review code helps more >>> than reducing load: it's a good opportunity for non-committers to get to >>> know the codebase (for both technical understandings and conventions), >>> interact with the community members, and make non-trivial contributions. >>> Reviewing code from a committer is especially valuable on all these points. >>> >>> - Robert >>> >>> >>> On Thu, May 31, 2018 at 11:35 AM Pablo Estrada <pabl...@google.com> wrote: >>>> >>>> In that case, does it make sense to say: >>>> >>>> - A code review by a committer is enough to merge. >>>> - Committers can have their PRs reviewed by non-committers that are >>>> familiar with the code >>>> - Non-committers may have their code reviewed by non-committers, but >>>> should have a committer do a lightweight review before merging. >>>> >>>> Do these seem like reasonable principles? >>>> -P. >>>> >>>> On Thu, May 31, 2018 at 11:25 AM Jean-Baptiste Onofré <j...@nanthrax.net> >>>> wrote: >>>>> >>>>> In that case, the contributor should be a committer pretty fast. >>>>> >>>>> I would prefer to keep at least a final validation from a committer to >>>>> guarantee the consistency of the project and anyway, only committer role >>>>> can merge a PR. >>>>> However, I fully agree that the most important is the Beam community. I >>>>> have no problem that non committer does the review and ask a committer >>>>> for final one and merge. >>>>> >>>>> Regards >>>>> JB >>>>> >>>>> On 31/05/2018 19:33, Andrew Pilloud wrote: >>>>> > If someone is trusted enough to review a committers code shouldn't they >>>>> > also be trusted enough to review another contributors code? As a >>>>> > non-committer I would get much quicker reviews if I could have other >>>>> > non-committers do the review, then get a committer who trusts us to >>>>> > merge. >>>>> > >>>>> > Andrew >>>>> > >>>>> > On Thu, May 31, 2018 at 9:03 AM Henning Rohde <hero...@google.com >>>>> > <mailto:hero...@google.com>> wrote: >>>>> > >>>>> > +1 >>>>> > >>>>> > On Thu, May 31, 2018 at 8:55 AM Thomas Weise <t...@apache.org >>>>> > <mailto:t...@apache.org>> wrote: >>>>> > >>>>> > +1 to the goal of increasing review bandwidth >>>>> > >>>>> > In addition to the proposed reviewer requirement change, perhaps >>>>> > there are other ways to contribute towards that goal as well? >>>>> > >>>>> > The discussion so far has focused on how more work can get done >>>>> > with the same pool of committers or how committers can get their >>>>> > work done faster. But ASF is really about "community over code" >>>>> > and in that spirit maybe we can consider how community growth >>>>> > can lead to similar effects? One way I can think of is that >>>>> > besides code contributions existing committers and especially >>>>> > the PMC members can help more towards growing the committer >>>>> > base, by mentoring contributors and helping them with their >>>>> > contributions and learning the ASF way of doing things. That >>>>> > seems a way to scale the project in the long run. >>>>> > >>>>> > I'm not super excited about the concepts of "owner" and >>>>> > "maintainer" often found in (non ASF) projects like Kenn >>>>> > mentions. Depending on the exact interpretation, these have the >>>>> > potential of establishing an artificial barrier and limiting >>>>> > growth/sustainability in the contributor base. Such powers tend >>>>> > to be based on historical accomplishments vs. current situation. >>>>> > >>>>> > Thanks, >>>>> > Thomas >>>>> > >>>>> > >>>>> > On Thu, May 31, 2018 at 7:35 AM, Etienne Chauchot >>>>> > <echauc...@apache.org <mailto:echauc...@apache.org>> wrote: >>>>> > >>>>> > Le jeudi 31 mai 2018 à 06:17 -0700, Robert Burke a écrit : >>>>> >> +1 I also thought this was the norm. >>>>> >> >>>>> >> My read of the committer/contributor guide was that a >>>>> >> committer couldn't unilaterally merge their own code >>>>> >> (approval/LGTM needs to come from someone familiar with >>>>> >> the component), rather than every review needs two >>>>> >> committers. I don't recall a requirement than each PR have >>>>> >> two committees attached, which I agree is burdensome >>>>> >> especially for new contributors. >>>>> > Yes me too, I thought exactly the same >>>>> >> >>>>> >> On Wed, May 30, 2018, 2:23 PM Udi Meiri <eh...@google.com >>>>> >> <mailto:eh...@google.com>> wrote: >>>>> >>> I thought this was the norm already? I have been the sole >>>>> >>> reviewer a few PRs by committers and I'm only a >>>>> >>> contributor. >>>>> >>> >>>>> >>> +1 >>>>> >>> >>>>> >>> On Wed, May 30, 2018 at 2:13 PM Kenneth Knowles >>>>> >>> <k...@google.com <mailto:k...@google.com>> wrote: >>>>> >>>> ++1 >>>>> >>>> >>>>> >>>> This is good reasoning. If you trust someone with the >>>>> >>>> committer responsibilities [1] you should trust them to >>>>> >>>> find an appropriate reviewer. >>>>> >>>> >>>>> >>>> Also: >>>>> >>>> >>>>> >>>> - adds a new way for non-committers and committers to >>>>> >>>> bond >>>>> >>>> - makes committers seem less like gatekeepers because >>>>> >>>> it goes both ways >>>>> >>>> - might help clear PR backlog, improving our community >>>>> >>>> response latency >>>>> >>>> - encourages committers to code* >>>>> >>>> >>>>> >>>> Kenn >>>>> >>>> >>>>> >>>> [1] >>>>> >>>> https://beam.apache.org/contribute/become-a-committer/ >>>>> >>>> >>>>> >>>> *With today's system, if a committer and a few >>>>> >>>> non-committers are working together, then when the >>>>> >>>> committer writes code it is harder to get it merged >>>>> >>>> because it takes an extra committer. It is easier to >>>>> >>>> have non-committers write all the code and the committer >>>>> >>>> just does reviews. It is 1 committer vs 2 being >>>>> >>>> involved. This used to be fine when almost everyone was >>>>> >>>> a committer and all working on the core, but it is not >>>>> >>>> fine any more. >>>>> >>>> >>>>> >>>> On Wed, May 30, 2018 at 12:50 PM Thomas Groh >>>>> >>>> <tg...@apache.org <mailto:tg...@apache.org>> wrote: >>>>> >>>>> Hey all; >>>>> >>>>> >>>>> >>>>> I've been thinking recently about the process we have >>>>> >>>>> for committing code, and our current process. I'd like >>>>> >>>>> to propose that we change our current process to >>>>> >>>>> require at least one committer is present for each code >>>>> >>>>> review, but remove the need to have a second committer >>>>> >>>>> review the code prior to submission if the original >>>>> >>>>> contributor is a committer. >>>>> >>>>> >>>>> >>>>> Generally, if we trust someone with the ability to >>>>> >>>>> merge code that someone else has written, I think it's >>>>> >>>>> sensible to also trust them to choose a capable >>>>> >>>>> reviewer. We expect that all of the people that we have >>>>> >>>>> recognized as committers will maintain the project's >>>>> >>>>> quality bar - and that's true for both code they author >>>>> >>>>> and code they review. Given that, I think it's sensible >>>>> >>>>> to expect a committer will choose a reviewer who is >>>>> >>>>> versed in the component they are contributing to who >>>>> >>>>> can provide insight and will also hold up the quality >>>>> >>>>> bar. >>>>> >>>>> >>>>> >>>>> Making this change will help spread the review load out >>>>> >>>>> among regular contributors to the project, and reduce >>>>> >>>>> bottlenecks caused by committers who have few other >>>>> >>>>> committers working on their same component. Obviously, >>>>> >>>>> this requires that committers act with the best >>>>> >>>>> interests of the project when they send out their code >>>>> >>>>> for reviews - but this is the behavior we demand before >>>>> >>>>> someone is recognized as a committer, so I don't see >>>>> >>>>> why that would be cause for concern. >>>>> >>>>> >>>>> >>>>> Yours, >>>>> >>>>> >>>>> >>>>> Thomas >>>>> > >>>>> > >>>>> >>>>> -- >>>>> -- >>>>> Jean-Baptiste Onofré >>>>> jbono...@apache.org >>>>> http://blog.nanthrax.net >>>>> Talend - http://www.talend.com >>>> >>>> -- >>>> Got feedback? go/pabloem-feedback
