geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dan Smith <dsm...@pivotal.io>
Subject Re: [DISCUSS] require reviews before merging a PR
Date Thu, 06 Jun 2019 16:26:17 GMT
>
> 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 <jmelchior@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 <jdeppe@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 <amurmann@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 <udo@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 <udo@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 <
> onichols@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>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message