+1 (binding) On 8 May 2017 at 17:45, Aled Sage <[email protected]> wrote:
> +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/4398448fd548495a5159016 >> a97afa12dd787ab34786b3bbc0881d5b4@%3Cdev.brooklyn.apache.org%3E >> >> Thanks >> Richard. >> >> >
