brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robert Moss <robert.m...@cloudsoft.io>
Subject Re: [VOTE] New standards for PR reviewing.
Date Mon, 08 May 2017 10:56:27 GMT
+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.
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message