> > Would it be possible to allow people who do not have committer status to > request reviewers on a pull request.
I'm not sure it's possible to do this with the "reviewers" field - if someone can figure out how to do this with the github IU, we can at least try filing an ticket with apache infrastructure. In the meantime, you can also just add a comment mentioning the people you'd like to review the request. That works for requesting reviews from non-committers as well. -Dan On Thu, Jun 6, 2019 at 9:05 AM Joris Melchior <jmelch...@pivotal.io> wrote: > Would it be possible to allow people who do not have committer status to > request reviewers on a pull request. In some cases we may know who should > take a look at it and in that case making it official by adding these > people to the pull request would be good IMO. > > On Thu, Jun 6, 2019 at 10:26 AM Jens Deppe <jde...@pivotal.io> wrote: > > > As reviewers we should also feel empowered to request additional > reviewers > > on a PR (perhaps beyond whomever the original submitter may already have > > requested). > > > > I think that, sometimes the complexity of a change prevents someone from > > commenting on just a portion of the change if they do not feel > comfortable > > understanding the scope of the whole change. > > > > Having said that though, once you have 'touched' a PR you should also be > > tracking the PR for additional commits or feedback until it is merged. > > > > --Jens > > > > On Wed, Jun 5, 2019 at 11:37 AM Alexander Murmann <amurm...@pivotal.io> > > wrote: > > > > > > > > > > If we trust committers, why review at all? Just commit... and we > might > > > > catch a problem, we might not. > > > > > > Honestly that to me would be the ideal state. However, our test > coverage > > > and code quality is nowhere near to allow for that. > > > > > > What I was referring to is different though. I didn't say "trust them > to > > > write perfect code", but trust " to decide how much review they require > > to > > > feel comfortable". In some cases this might mean one review and in > > others > > > maybe two, three or even more and maybe even by very specific people. > > > > > > On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <u...@apache.org> wrote: > > > > > > > Alexander, thank you for your response. And yes, change is > > uncomfortable > > > > and in some cases more reviewers would not have caught issues. BUT, > > more > > > > people would have seen the code, maybe become more familiar with it, > > > etc... > > > > > > > > I don't say don't trust committers, as I am one. But I also know > that I > > > > mistakes are made regardless of intent. If we trust committers, why > > > > review at all? Just commit... and we might catch a problem, we might > > not. > > > > > > > > --Udo > > > > > > > > On 6/5/19 11:20, Alexander Murmann wrote: > > > > > Udo, I agree with many of the pains you are addressing, but am > > > > pessimistic > > > > > that having more reviewers will solve them. > > > > > > > > > > You are absolutely correct in calling out that the code is ugly > > complex > > > > and > > > > > missing coverage. The best way to address this is to clean up the > > code > > > > and > > > > > improve coverage. You say yourself "In the past single small > changes > > > have > > > > > caused failures the were completely unforeseen by anyone". I don't > > > think > > > > > more eyeballs will go a long way in making someone see complex bugs > > > > > introduced by seemingly safe changes. > > > > > > > > > > I also am concerned that introducing a hurdle like this will make > > > > > committers more excited to review PRs with care, but rather might > > lead > > > to > > > > > less care. It would be great of our committers were more > passionate > > > > about > > > > > PR reviews and do them more often, but forcing it rarely > accomplishes > > > > that > > > > > goal. > > > > > > > > > > I'd rather see us trust our committers to decide how much review > they > > > > > require to feel comfortable about their work and use the time saved > > to > > > > > address the root of the problem (accidental complexity & lack of > test > > > > > coverage) > > > > > > > > > > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <u...@apache.org> > > wrote: > > > > > > > > > >> @Kirk, I totally understand the pain that you speak of. I too > agree > > > that > > > > >> every line of changed code should have a test confirming that > > behavior > > > > >> was not changed. > > > > >> > > > > >> I don't believe that we need to go as far as revoking committer > > rights > > > > >> and reviewing each committer again, BUT it would be AMAZING that > out > > > of > > > > >> our 100 committers, 80% of them would be more active in PR > reviews, > > > > >> mailing lists and in the end active on the project outside of > their > > > > >> focus area. > > > > >> > > > > >> I do want to remind all Geode committers, it IS your > responsibility > > to > > > > >> be part of the PR review cycle. I will hold myself just as > > accountable > > > > >> to this than what I hold every committer to, as I've been just as > > lazy > > > > >> as the rest of them. > > > > >> > > > > >> BUT > > > > >> > > > > >> The reality is: > > > > >> > > > > >> 1. Geode code is HUGELY complex and NOT a test complete as we'd > > like > > > > >> 2. In the past single small changes have caused failures the > were > > > > >> completely unforeseen by anyone > > > > >> 3. In the past commits with single reviewers, have caused > backward > > > > >> compatibility issues which were only caught by chance in > > > unrelated > > > > >> testing. > > > > >> 4. There are 100 committers on Geode, and we keep on arguing > that > > it > > > > is > > > > >> hard to get PR's reviewed and that is why it is ok to have > > only 1 > > > > >> reviewer per PR. > > > > >> 5. There seems to be majority (unstated) opinion of: "why > change, > > it > > > > >> has been working for us so far." (I call is unstated, because > > > being > > > > >> silent means you agree with the status quo) > > > > >> 6. With requiring only 1 reviewer on code submissions, we are > > > possibly > > > > >> creating areas of the code only understood by a few. > > > > >> > > > > >> IF, we as a project, have decided that all code shall enter only > > > through > > > > >> the flow of PR, then why not extend the QA cycle a little by > > requiring > > > > >> more eyes. Lazy consensus, is as stated, lazy and would only work > > in a > > > > >> project where the levels of complexity and size are not as high as > > > > >> Geode's. In addition, with PR submissions, we have admitted that > we > > > are > > > > >> human and could make mistakes and in an already complex > environment > > > and > > > > >> to the best of our ability get it wrong. > > > > >> > > > > >> Now, there are commits that really do not require 3 pairs of eyes, > > > > >> because spelling mistakes and typos don't need consensus. But any > > time > > > > >> code logic was amended, this needs to be reviewed. > > > > >> > > > > >> I have seen different approach to code submissions: > > > > >> > > > > >> * The submitter of the PR is NOT the committer of the PR. This > > task > > > > is > > > > >> the responsibility of another committer(s) to review, approve > > and > > > > >> finally merge in. > > > > >> * Smaller amount of committers with higher numbers of > > contributors. > > > > >> Yes, this does create a bottleneck, but it promotes a sense > of > > > > pride > > > > >> and responsibility that individual feels. Possibly a greater > > > > >> understanding of the target module is promoted through this > > > > approach > > > > >> as well. > > > > >> > > > > >> Now, I don't say we as a project should follow these strict or > > > > >> restricting approaches, but from my perspective, if we as a > project > > > > >> argue that we struggle to find 3 reviewers out of 100, then there > > are > > > > >> bigger problems in the project than we anticipated. It is not a > lack > > > of > > > > >> trust in our committers, to me it is a sense of pride that I want > > > other > > > > >> committers to confirm that I've delivered code to the high > standard > > > that > > > > >> we want to be known for. Whilst it is painful to go through the > > > process, > > > > >> but if done correctly it is beneficial to all involved, as > differing > > > > >> opinions and approaches can be shared and all can learn from. > > > > >> > > > > >> In addition, I have personally stumbled upon a few PR's, which > upon > > > > >> review found to be lacking in the areas of best practices of code > > > and/or > > > > >> design. > > > > >> > > > > >> I fully support the notion of 3 reviewers per PR. I'm also going > to > > > take > > > > >> it one step further, in the list of reviewers, there is at least 1 > > > > >> reviewer that is not part of a team, as this might drive a > unbiased > > > view > > > > >> of the code and approach. I would also like to encourage ALL > > > committers > > > > >> to review code outside of the focus area. This will only promote a > > > > >> broader understanding of the project codebase. I also support the > > > notion > > > > >> of a pair/mobbing reviews, if a reviewer does not understand the > > > problem > > > > >> area enough to effectively review, OR where the solution is not > > > > apparent. > > > > >> > > > > >> --Udo > > > > >> > > > > >> On 6/4/19 16:49, Kirk Lund wrote: > > > > >>> 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. > > > > >>>> > > > > >>>> > > > > > > > > > > > > -- > *Joris Melchior * > CF Engineering > Pivotal Toronto > 416 877 5427 > > “Programs must be written for people to read, and only incidentally for > machines to execute.” – *Hal Abelson* > <https://en.wikipedia.org/wiki/Hal_Abelson> >
