my two cents (although Im a newcomer here):
I agree with Kirk in the point that if a PR is sent by a committer, if after "grace period" (one/two weeks?) no one reviewed it, the author could merge it. But of course he/she is free to wait for a review. I was thinking about PRs made by contributors, as it is my case. A possible approach could be that if the author is a contributor, then at least one review is needed. Once that review is done, the reviewer should be free to decide between merging the change or, if the PR is still under the "grace period", keep it open waiting for other reviews. If the review is done after the "grace period", then the reviewer should merge the change. And if a contributor PR has at least a review and the "grace period" ends, the author is allowed to ask someone for merge it, as it already has the green light from a committer. Finally, I also agree on the point of increasing test coverage and follow clean code principles. ________________________________ De: Kirk Lund <kl...@apache.org> Enviado: miércoles, 5 de junio de 2019 1:49:06 Para: geode Asunto: Re: [DISCUSS] require reviews before merging a PR I'm -1 for requiring N reviews before merging a commit. Overall, I support Lazy Consensus. If I post a PR that fixes the flakiness in a test, the precheckin jobs prove it, and it sits there for 2 weeks without reviews, then I favor merging it in at that point without any reviews. I'm not going to chase people around or spam the dev list over and over asking for reviews. Nothing in the Apache Way says you have to do reviews before committing -- some projects prefer "commit then review" instead of "review then commit". You can always look at the code someone changed and you can always change it further or revert it. I think if we don't trust our committers then we have a bigger systemic problem that becoming more strict about PR reviews isn not going to fix. Overall, I also favor pairing/mobbing over reviews. Without being there during the work, a reviewer lacks the context to understand why it was done the way it was done. If we cannot establish or maintain trust in committers, then I think we should remove committer status from everyone and start over as a project, proposing and accepting one committer at a time. Instead of constraints on reviews, I would prefer to establish new criteria for coding such as: 1) all classes touched in a PR must have a unit test created if none exists 2) all code touched in a PR must have unit test coverage (and possibly integration test coverage) specific to the changes 3) all new classes must have full unit test coverage 4) all code touched in a PR must follow clean code principles (which would obviously need defining on the wiki) Then it becomes the responsibility of the author(s) and committer(s) of that PR to ensure that the code and the PR follows the project's criteria for code quality and test coverage. It also becomes easier to measure the PRs of a non-committer to determine if we think they would make a good committer (for example, do they adhere to clean code quality and unit testing with mocks? -- along with any other criteria). On Thu, May 30, 2019 at 3:51 PM Owen Nichols <onich...@pivotal.io> wrote: > It seems common for Geode PRs to get merged with only a single green > checkmark in GitHub. > > According to https://www.apache.org/foundation/voting.html we should not > be merging PRs with fewer than 3 green checkmarks. > > Consensus is a fundamental value in doing things The Apache Way. A single > +1 is not consensus. Since we’re currently discussing what it takes to > become a committer and what standards a committer is expected to uphold, it > seems like a good time to review this policy. > > GitHub can be configured to require N reviews before a commit can be > merged. Should we enable this feature? > > -Owen > VOTES ON CODE MODIFICATION < > https://www.apache.org/foundation/voting.html#votes-on-code-modification> > For code-modification votes, +1 votes are in favour of the proposal, but > -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto> > and kill the proposal dead until all vetoers withdraw their -1 votes. > > Unless a vote has been declared as using lazy consensus < > https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1 > votes are required for a code-modification proposal to pass. > > Whole numbers are recommended for this type of vote, as the opinion being > expressed is Boolean: 'I approve/do not approve of this change.' > > > CONSENSUS GAUGING THROUGH SILENCE < > https://www.apache.org/foundation/voting.html#LazyConsensus> > An alternative to voting that is sometimes used to measure the > acceptability of something is the concept of lazy consensus < > https://www.apache.org/foundation/glossary.html#LazyConsensus>. > > Lazy consensus is simply an announcement of 'silence gives assent.’ When > someone wants to determine the sense of the community this way, it might do > so with a mail message such as: > "The patch below fixes GEODE-12345; if no-one objects within three days, > I'll assume lazy consensus and commit it." > > Lazy consensus cannot be used on projects that enforce a > review-then-commit < > https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy. > > >
