+1 (binding)
Thanks all for taking this seriously. Just to recap (and apologies for
doing this in a VOTE thread but I'll keep it brief) my aim is to
increase the priority with which we review things, not to downplay the
importance of reviews, though where something has to give I'd rather it
be review depth not review speed.
Best
Alex
On 08/05/2017 12:07, Duncan Godwin wrote:
+1 (binding)
On 8 May 2017 at 12:04, Geoff Macartney <[email protected]>
wrote:
+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.
