> > 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. >
According to their docs [1], "collaborator with write access" is the GitHub required criteria to add reviewers. So, yeah, you have to be a committer. If you're not a committer, you could always request in your PR comments for a committer to add some specific people, should you have any in mind. Imagination is Change. ~Patrick [1] https://help.github.com/en/articles/requesting-a-pull-request-review On Thu, Jun 6, 2019 at 9:26 AM Dan Smith <dsm...@pivotal.io> wrote: > > > > 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> > > >
