[DISCUSS] What Reviews Required-or-Needed / When OK to merge?

[Austin Bennett <wh...@gmail.com>]

Hi Flagon-Dev,

Do we have a Flagon community standard for when OK to merge a PR?

Inviting us to have a discussion, and to get consensus around community
standards for reviewing PRs/when-OK-to-Merge.  Apologies if I missed a
prior discussion/doc on this topic.

I assume we ultimately need a committer to review the work of a
non-committer ahead of merge ( since someone with permissions eventually
has to actually accept/merge/commit the code ).  In general, is one
reviewer/committer sufficient?

How does this change if the author of the PR is an existing committer?  I
have seen some communities require a committee to review things ( no matter
the status of the author), and others require anyone else -- so that's more
of a community determination.  For example, could also be a judgement call
as to whether to tag in the expertise of someone having great depth on a
specific component, or just some eyeballs for sanity check.

Hoping we can try to be *somewhat* specific [ where possible ] about the
sort of expected requirements, and our standards.  A result of the
discussion likely will be things that can be summarized and used for
updating http://flagon.incubator.apache.org/docs/contributing/

And, naturally there are exceptions ( ex: very large commits, when
functionality significantly altered [ such a case could warrant a FIP /
Flagon-Improvement-Proposal ], etc ).  And, exception in the other
direction -- how sufficiently small to be OK merging without another
reviewer [ of any kind ]?

Thanks,
Austin

Re: [DISCUSS] What Reviews Required-or-Needed / When OK to merge?

[Joshua Poore <po...@apache.org>]

Great thread!

RE:
> *I assume we ultimately need a committer to review the work of
> anon-committer ahead of merge ( since someone with permissions
> eventuallyhas to actually accept/merge/commit the code ).  In general, is
> onereviewer/committer sufficient?*
> IMO, yes, one reviewer who is also a committer is sufficient
> 
> *How does this change if the author of the PR is an existing committer?*
> If I am a committer and I make a PR, I would wait for another committer to
> review and approve. However, if it's been a week and no one approves, I
> would send a review request to dev@, and if there is no response after a
> couple of weeks, then I'd go ahead and merge it myself (given all
> tests/checks have passed of course)
> 
> Thanks for the discussion!
> - Gedd

I like Gedd’s pragmatic take on this. All PR’s *should* get a review, whether from a committer or contributor. 

I will add that it probably should be PPMC that reviews (not all committers are PPMC), until we have some set of designated release managers that may or may not be PPMC.

I agree with Rob that one review should be sufficient.

There are some alt-cases that merit discussion, such as: 

1. whether we need to review a PR if it comes from a committer, and it incorporates only minor changes to update package dependencies (basic maintenance)? Should all basic maintenance be handled via PR? That seems extreme to me...

2. how to handle merges during release procs by a release manager? Right before a VOTE, there are always a bunch of tiny things—DRAT/Licensing, README corrections, URLs, etc. Should those be handled by PR?

I think a Test branch takes care of a lot of this: new PRs and merges made against a test branch, then maybe independent  PPMC reviews and merges with Master? 


Great thread! Thanks for starting Austin. Sorry I’m late to the party!

Josh

Re: [DISCUSS] What Reviews Required-or-Needed / When OK to merge?

[Gedd Johnson <ge...@gmail.com>]

*I assume we ultimately need a committer to review the work of
anon-committer ahead of merge ( since someone with permissions
eventuallyhas to actually accept/merge/commit the code ).  In general, is
onereviewer/committer sufficient?*
IMO, yes, one reviewer who is also a committer is sufficient

*How does this change if the author of the PR is an existing committer?*
If I am a committer and I make a PR, I would wait for another committer to
review and approve. However, if it's been a week and no one approves, I
would send a review request to dev@, and if there is no response after a
couple of weeks, then I'd go ahead and merge it myself (given all
tests/checks have passed of course)

Thanks for the discussion!
- Gedd

Re: [DISCUSS] What Reviews Required-or-Needed / When OK to merge?

[Rob Foley <rf...@apache.org>]

I don't have a strong opinion on this but, in my experience, having one 
reviewer ought to be sufficient assuming they are familiar with the 
code-to-be-changed. I don't believe committers should be able review 
their code outside of extraordinary circumstances, such as a very high 
priority security issue with no available reviewers. Another alternative 
is to actually set up a system of "code owners" for different 
components, but maintaining such a list (and reducing your pool of 
reviewers) can be a bit tiresome.

Regards,
Rob

On 6/14/2022 3:39 PM, Austin Bennett wrote:
> Hi Flagon-Dev,
>
> Do we have a Flagon community standard for when OK to merge a PR?
>
> Inviting us to have a discussion, and to get consensus around community
> standards for reviewing PRs/when-OK-to-Merge.  Apologies if I missed a
> prior discussion/doc on this topic.
>
> I assume we ultimately need a committer to review the work of a
> non-committer ahead of merge ( since someone with permissions eventually
> has to actually accept/merge/commit the code ).  In general, is one
> reviewer/committer sufficient?
>
> How does this change if the author of the PR is an existing committer?  I
> have seen some communities require a committee to review things ( no matter
> the status of the author), and others require anyone else -- so that's more
> of a community determination.  For example, could also be a judgement call
> as to whether to tag in the expertise of someone having great depth on a
> specific component, or just some eyeballs for sanity check.
>
> Hoping we can try to be *somewhat* specific [ where possible ] about the
> sort of expected requirements, and our standards.  A result of the
> discussion likely will be things that can be summarized and used for
> updating http://flagon.incubator.apache.org/docs/contributing/
>
> And, naturally there are exceptions ( ex: very large commits, when
> functionality significantly altered [ such a case could warrant a FIP /
> Flagon-Improvement-Proposal ], etc ).  And, exception in the other
> direction -- how sufficiently small to be OK merging without another
> reviewer [ of any kind ]?
>
> Thanks,
> Austin
>