So far I would like to thank everyone for their thoughts and input.
@Dave, I would love to find a solution to the partial sign-off. I’ve been
experimenting with the “Projects” setting. I wonder if we cannot have a
“Documentation Check” project, that is added to every PR as a default project.
We could have different states with the project, which would allow the docs
folk to know what PRs are new and which still need to be reviewed for docs
changes.
Now, I don’t know if we can restrict the merging of a PR based upon a state in
the Project, but at the very least it will provide the ability to have an
overview of PR with/without docs review. You can have a look at the “Quality
Review” project I have created. Which I use to track all PRs that I would like
to review for quality purposes. (code, structure, tests, etc)… I think Docs
could have something similar.
@Bruce, I’m not trying to create another rule for the sake of creating a rule.
Why do you believe that we as a community will give any submitter a stink-eye
just because they did not submit a draft? I certainly would not. I would
suggest that the submitter maybe submit a draft IF the PR is not in a ready
state and needs a few more iterations to get to a ready state.
I believe it is easier and better for committers to go through a list of PRs to
review if they know that the PR passes all of the testing checks.. As a failure
in one area might actually cause some code components to change. Which might
void an earlier review of the code. Also, I’m not suggesting that there are no
reviews before the commit checks go green. You can easily request someone else
to review whilst in a draft state.
As for knowing what reviewers to tag for a review is more limiting. How would I
as a new PR submitter know WHO I should tag in the PR? Over time we have built
up a great understanding of who might be a good person to review our code. But
for a new community member, they do not know this. For them, they submit the
PR, and someone in the community will review it.
I would also like everyone to think back on their own approach on deciding what
PRs to review.
Do you look at the PR and decide to wait until all commit checks are green?
Do you go through the list and find one, that you think you can review, whilst
the commit checks are still running?
Do you only review PRs in which you have been explicitly tagged?
Do you scan the PRs for a commit in an area of “expertise”?
Do you scan the PRs for committers that you know?
Whatever approach we take, I would like us to come up with an approach, that we
as a community follow, to have a consistent approach to the review.
A consistent way we can evaluate if the code is in a “ready” state?
A consistent way, that the community will know, that when they submit the PR it
will be looked at.
A consistent way that I, as a committer, will know that if I spend the time to
review the PR will not be a waste of my time, because it wasn’t ready.
I don’t think community members are repulsed by a project with structure, but I
do know that I question a project without structure and one where it takes a
long time to have a PR reviewed.
I would also wlecome our newer community members (anyone who has been a
committer for less than 18months or anyone who is not a committer) to comment
here. Is there anything in particular that attracts or repulses you when it
comes to contributing to the project.
--Udo
From: Dave Barnes <dbar...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: PR process and etiquette
Here's a common use case that we should address: A single PR may require
two reviews, one for code and another for docs, before it can be said to be
fully reviewed and ready to merge.
Points to consider:
- Many PRs, especially those introducing new features or user-settable
properties, include both code and docs. "Docs" includes updates to the user
guide sources, but also code comments that are consumed by community
developers and are included in the auto-generated API documentation.
Parameter name choices, explanations, spelling and grammar need to be right.
- Reviews of code and docs require different skills, so a good-quality
PR review in such cases benefits from multiple reviewers, (at least) one
for code, and (at least) one for docs.
- [My key concern] Current protocol is 1 reviewer approval qualifies the
PR for merging. I flag my doc reviews in such cases with a note to the
submitter saying something like "Docs reviewed and approved; technical
review still needed before merging". But a submitter who's eager to merge
may just see green and go for it, without reading the caveat. I'm against
enforcing a multiple-review requirement. What I would like to see is a
status for partial approval, so I can register my approval without
prematurely opening the door to a code merge.
- Other considerations:
- There are PRs that don't fit this two-part use case. Many PRs are
code-only, with no doc component. Conversely, some PRs are
docs-only, with
no code component.
- PRs often are not recognized as falling into this two-part use
case. Submitters of JIRA tickets may not know whether the solution to the
problem they're reporting will affect docs. Submitter of the PR
addressing
the JIRA ticket may not think to add the 'docs' component to the related
JIRA ticket or to request a doc reviewer when creating the PR.
On Wed, Oct 28, 2020 at 8:41 AM Robert Houghton <rhough...@vmware.com>
wrote:
> There are some pieces of Apache infrastructure we can control without
> needing tickets: Specifically
> required_pull_request_reviews:
> dismiss_stale_reviews: true
> require_code_owner_reviews: true
>
> I think these specific controls could go a long way towards helping keep
> our PR quality high, while reducing cognitive load for the reviewers.
>
> Full documentation here<
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FINFRA%2Fgit%2B-%2B.asf.yaml%2Bfeatures%23git.asf.yamlfeatures-BranchProtection&data=04%7C01%7Cudo%40vmware.com%7Cb24abab8b1de4fb941e908d87b65bf85%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637395024222714248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dfcztCMeKyqQPLhe9ox51IA7ge2nqpE39OxRdyoNSHg%3D&reserved=0
> >
>
>
>
> From: Bruce Schuchardt <bru...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <dev@geode.apache.org>
> Subject: Re: PR process and etiquette
> -1
> While I often use the Draft option I don't see why we want to add even
> more rules about how we use github. I think it's enough to put in a PR and
> then add reviewers when you're ready for comments. Getting the stink-eye
> for putting up a non-Draft PR is just going to make it more difficult to
> attract and retain new contributors.
>
> On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <u...@vmware.com> wrote:
>
> Dear Apache Geode Devs,
> It is really great going through all the PRs that been submitted. As
> Josh Long is known to say: "I work for PRs".
> Whilst going through some of the PRs I do see that there are many PRs
> that have multiple commits against the PR.
> I know that the PR submission framework kicks off more testing than we
> do on our own local machines. It is extremely uncommon to submit a PR the
> first time and have all tests go green. Which h means we invariably iterate
> over commits to make the build go green.
> In this limbo time period, it is hard for any reviewer to know when
> the ticket is ready to be reviewed.
> I want to propose that when submitting a PR, it is initially submitted
> as a DRAFT PR. This way, all test can still be run and work can be done to
> make sure "green" is achieved. Once "green" status has been achieved, the
> draft can then be upgraded to a final PR by pressing the "Ready For Review"
> button. At this point all commits on the branch can then once again be
> squashed into a single commit.
> Now project committers will now know that the PR is in a state that it
> can be reviewed for completeness and functionality.
> In addition, it will help tremendously helpful if anyone submitting a
> PR monitors their PR for activity. If there is no activity for a few days,
> please feel free to ping the Apache Geode Dev community for review. If
> review is request, please prioritize some time to address the feedback, as
> reviewers spend time reviewing code and getting an understanding what the
> code is doing. If too much time goes by, between review and addressing the
> review comments, not only does the reviewer lose context, possibly
> requiring them to spend time again to understand what the code was/is
> supposed to do, but also possibly lose interest, as the ticket has now
> become cold or dropped down the list of PRs.
> There are currently many PRs that are in a cold state, where the time
> between review and response has been so long, that both parties (reviewer
> and submitter) have forgotten about the PR.
> In the case that the reviews will take more time to address than
> expected, please downgrade the PR to draft status again. At this point, it
> does not mean that reviewers will not be able to help anymore, as you can
> request a reviewer to help with feedback and comments, until one feels that
> the PR is back in a state of final submission.
> So, what I'm really asking from the Dev Community:
> If you submit a PR, it would be great if you can nudge the
> community if there is no review on the PR. If feedback is provided on a PR,
> please address it as soon as possible. This not only will help the PR
> progress faster, but it will shorten the feedback loop.
> Finally, start with a DRAFT PR and then upgrade to a final PR
> when you feel it is in a "good" state. If more work is required, it is ok
> to downgrade back to a draft, until one feels the PR is in a good state
> again.
> Managing the PR process in this manner, will be hugely beneficial for
> all community members. As reviewers will know when a PR is ready to be
> reviewed. Reviewers will also feel more engaged in this process, due to
> timely response to their review comments. PR submitters will feel happier,
> in the knowledge that the code they spent so long meticulously crafting
> will possibly make it into the project.
> Any thoughts?
> --Udo
>
>
