geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Patrick Rhomberg <prhomb...@apache.org>
Subject Re: [DISCUSS] require reviews before merging a PR
Date Thu, 06 Jun 2019 16:40:16 GMT
>
> 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 <dsmith@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 <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