+1 (binding)
On 08/05/2017 11:55, Richard Downer 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/4398448fd548495a5159016a97afa12dd787ab34786b3bbc0881d5b4@%3Cdev.brooklyn.apache.org%3E Thanks Richard.
