Mailing-List: contact dev-help@brooklyn.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@brooklyn.apache.org
MIME-Version: 1.0
In-Reply-To: <CAE7C0PdH0bnArSmpGwpvNbKQkJCz7j5HJROSo0nsTSmts7Dueg@mail.gmail.com>
References: <CABQFKi2sYnGOdE5hxNbcHyg+PWq1kSQtstvF+ahinYhb5zS1ZQ@mail.gmail.com>
 <CABQFKi2sa5ASquPAhxBWJ+cGvVE4jv=utn9cb7VXhFu=2KjVBQ@mail.gmail.com>
 <CALuNGfsXiFo1P0EkLMGxLTnN2UmEhjSLqd+jRL7wZjsPNU3xjA@mail.gmail.com> <CAE7C0PdH0bnArSmpGwpvNbKQkJCz7j5HJROSo0nsTSmts7Dueg@mail.gmail.com>
From: Duncan Godwin <duncan.godwin@cloudsoftcorp.com>
Date: Mon, 8 May 2017 12:07:42 +0100
Message-ID: <CABXp0fRYkghU1=Uj1NSMd9LfaJh3g6-wqxdOZcRkUnBQu6WRXw@mail.gmail.com>
Subject: Re: [VOTE] New standards for PR reviewing.
To: dev@brooklyn.apache.org
Content-Type: multipart/alternative; boundary=f403045ec2da80faf1054f013fba
archived-at: Mon, 08 May 2017 11:07:53 -0000

--f403045ec2da80faf1054f013fba
Content-Type: text/plain; charset=UTF-8

+1 (binding)

On 8 May 2017 at 12:04, Geoff Macartney <geoff.macartney@cloudsoftcorp.com>
wrote:

> +1 (binding)
>
> On Mon, 8 May 2017 at 11:56 Robert Moss <robert.moss@cloudsoft.io> wrote:
>
> > +1
> >
> > On 8 May 2017 at 11:55, Richard Downer <richard@apache.org> wrote:
> >
> > > +1 (binding)
> > >
> > >
> > > On 8 May 2017 at 11:55, Richard Downer <richard@apache.org> 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.
> > > >
> > > >
> > >
> >
>

--f403045ec2da80faf1054f013fba--