+1 (binding) On Mon, 8 May 2017 at 11:56 Robert Moss <[email protected]> wrote:
> +1 > > On 8 May 2017 at 11:55, Richard Downer <[email protected]> wrote: > > > +1 (binding) > > > > > > On 8 May 2017 at 11:55, Richard Downer <[email protected]> wrote: > > > > > There have been recent discussions about how the committers assess PRs > > for > > > merging. The discussion is summarised below and the original thread > > > available at [1]. > > > > > > The consensus of the discussion is to adopt new standards for > committers > > > reviewing PRs, as follows: > > > > > > ------ > > > > > > If a PR has not been reviewed within a certain amount of time - > suggested > > > to be 7 days, or less for smaller PRs - nor has a committer indicated > > that > > > they are doing a detailed review, then the PR shall be considered for a > > > less detailed review, an "eyeball test". > > > > > > Under an eyeball test, a reviewer will consider if the PR is: > > > * clearly helpful & not obviously wrong > > > * low-risk / doesn't break compatibility > > > * good test coverage (and passing) > > > * likely to be maintained > > > > > > If it passes the above criteria, then the reviewer will add a comment > to > > > the PR, and ask if further review is appropriate, possibly tagging > > specific > > > committers who may be interested. Then if there is no objection within > 72 > > > hours, passive consensus should be assumed, and the PR merged. > > > > > > If the PR does not pass the above criteria, the reviewer should say > what > > > they have doubts about, suggest what the contributor could do to help, > > > and/or appeal to other committers more familiar with an area. If > > > appropriate, move from GitHub onto the mailing list. (The aim here is > to > > > get a discussion going and not give the contributor the impression > their > > PR > > > is being ignored.) > > > > > > ------ > > > > > > This vote is to decide if we wish to adopt these standards for all PR > > > reviews going forward, and to document these standards in our website. > > > > > > This vote will be open for a minimum of 72 hours. > > > > > > [ ] +1 - adopt this standard > > > [ ] 0 - no opinion > > > [ ] -1 - do not adopt this standard, because: > > > > > > ------ > > > > > > Background: > > > > > > This is related to the recent thread at [1]. > > > > > > Traditionally, this project has had a high bar for reviewing > > contributions > > > prior to merging. This dates back to the project's inception, before it > > was > > > part of Apache. Reviewers would be expected to inspect the code and > > > personally test it before allowing it to be merged. > > > > > > There has been concern expressed that this is holding back Brooklyn > > > development. Reviewing a PR can be time-consuming; often a detailed > > review > > > requires expert knowledge in a particular area of the code which only > > some > > > committers possess. The result is that PRs, especially larger ones or > > ones > > > in core areas of the project, do not receive timely review, and in some > > > cases languish far too long. This is bad for the project as it holds > back > > > our velocity, and frustrates contributors who see their changes stuck > in > > > the system for extended lengths of time. > > > > > > Since we joined the ASF, we have had feedback from others with > experience > > > in Apache that we are too conservative with our code review > requirements. > > > We also recognise the value in automated testing to catch regressions > > > (although these constantly need work, of course), and in our Git source > > > control to enable us to revert changes that turn out to be particularly > > > problematic. We can relax our strict reviewing requirements, which will > > > increase our velocity, and show our contributors that their work is > > > receiving attention and getting merged. Should a merge prove to be > > > problematic, their is still opportunity to do a bug fix (and get it > > merged > > > under the same fast process, too), and ultimately the chance to > "revert" > > > the merge if necessary. > > > > > > So we believe that the quality of the finished product will not be > > > adversely affected by these changes. > > > > > > > > > [1]https://lists.apache.org/thread.html/4398448fd548495a5159016a97afa1 > > > 2dd787ab34786b3bbc0881d5b4@%3Cdev.brooklyn.apache.org%3E > > > > > > Thanks > > > Richard. > > > > > > > > >
