You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Udo Kohlmeyer <ud...@vmware.com> on 2020/10/28 00:41:11 UTC
PR process and etiquette
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 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
Re: PR process and etiquette
Posted by Kirk Lund <kl...@apache.org>.
How about we make this a recommendation rather than a rule?
I'd like to also recommend that contributors consider prefixing the PR
title with "DRAFT: " while it is in draft. This just makes it easier to see
at a glance that it's a draft. When I change the PR to "ready for review" I
edit the title to remove "DRAFT: ".
On Wed, Oct 28, 2020 at 9:59 AM Bruce Schuchardt <br...@vmware.com> wrote:
> Hi Owen - I wasn't aware that non-committers can't add reviewers to their
> PRs but I don't see how using DRAFT mode helps with that. The idea that I
> can't request a review until the commit checks all pass seems absurd to me.
>
> On 10/28/20, 9:15 AM, "Owen Nichols" <on...@vmware.com> wrote:
>
> Hey Bruce, please consider that non-committers are not permitted to
> add reviewers themselves, so a consistent convention to indicate when a PR
> has moved from work-in-progress to ready-for-review will help alert the
> community when to assign reviewers.
>
> Currently, I see countless creative variations on people adding "WIP"
> or "DO NOT REVIEW" or other text somewhere in the summary or description.
> I think the suggestion here is to standardize on using Draft status as the
> canonical way to communicate this.
>
> Also worth noting, you can change a PR back to 'draft' mode at any
> time (so whether you forgot initially, or a test failure or review feedback
> made you realize more work is needed, you can always amend the draft status
> to communicate whether you're still working, or ready for review again).
>
> I'm not sure why you think this is going to make it more difficult to
> attract and retain new contributors. The point is to make it easier for
> new contributors to get their PRs reviewed, and give reviewers more
> confident that their time is appreciated.
>
> I see your point that as a committer, you have no need to communicate
> this status, since you can simply add reviewers yourself when you're
> ready. Asking everyone to use draft status is not the ideal workaround for
> the GitHub flaw that non-committers can't request reviewers, but it also
> takes only 2 seconds of your time. Expecting new contributors to email the
> dev list every time they want to request a review of their PR seems far
> more problematic in term of attracting and retaining new contributors.
>
> On 10/28/20, 8:10 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
>
> -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" <ud...@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
>
>
>
>
>
Re: PR process and etiquette
Posted by Bruce Schuchardt <br...@vmware.com>.
Hi Owen - I wasn't aware that non-committers can't add reviewers to their PRs but I don't see how using DRAFT mode helps with that. The idea that I can't request a review until the commit checks all pass seems absurd to me.
On 10/28/20, 9:15 AM, "Owen Nichols" <on...@vmware.com> wrote:
Hey Bruce, please consider that non-committers are not permitted to add reviewers themselves, so a consistent convention to indicate when a PR has moved from work-in-progress to ready-for-review will help alert the community when to assign reviewers.
Currently, I see countless creative variations on people adding "WIP" or "DO NOT REVIEW" or other text somewhere in the summary or description. I think the suggestion here is to standardize on using Draft status as the canonical way to communicate this.
Also worth noting, you can change a PR back to 'draft' mode at any time (so whether you forgot initially, or a test failure or review feedback made you realize more work is needed, you can always amend the draft status to communicate whether you're still working, or ready for review again).
I'm not sure why you think this is going to make it more difficult to attract and retain new contributors. The point is to make it easier for new contributors to get their PRs reviewed, and give reviewers more confident that their time is appreciated.
I see your point that as a committer, you have no need to communicate this status, since you can simply add reviewers yourself when you're ready. Asking everyone to use draft status is not the ideal workaround for the GitHub flaw that non-committers can't request reviewers, but it also takes only 2 seconds of your time. Expecting new contributors to email the dev list every time they want to request a review of their PR seems far more problematic in term of attracting and retaining new contributors.
On 10/28/20, 8:10 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
-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" <ud...@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
Re: PR process and etiquette
Posted by Owen Nichols <on...@vmware.com>.
For the narrow goal of making it easier for non-committers to get reviewers for their PRs, we could also consider defining a "reviewers wanted" label. However it might not be very obvious to new committers that they need to click the gear to look for that label, unless we also update the PR template text to mention it.
Using lack-of-draft-status as the ready-for-review indicator does have the advantage of being the 'default' state.
On 10/28/20, 9:15 AM, "Owen Nichols" <on...@vmware.com> wrote:
Hey Bruce, please consider that non-committers are not permitted to add reviewers themselves, so a consistent convention to indicate when a PR has moved from work-in-progress to ready-for-review will help alert the community when to assign reviewers.
Currently, I see countless creative variations on people adding "WIP" or "DO NOT REVIEW" or other text somewhere in the summary or description. I think the suggestion here is to standardize on using Draft status as the canonical way to communicate this.
Also worth noting, you can change a PR back to 'draft' mode at any time (so whether you forgot initially, or a test failure or review feedback made you realize more work is needed, you can always amend the draft status to communicate whether you're still working, or ready for review again).
I'm not sure why you think this is going to make it more difficult to attract and retain new contributors. The point is to make it easier for new contributors to get their PRs reviewed, and give reviewers more confident that their time is appreciated.
I see your point that as a committer, you have no need to communicate this status, since you can simply add reviewers yourself when you're ready. Asking everyone to use draft status is not the ideal workaround for the GitHub flaw that non-committers can't request reviewers, but it also takes only 2 seconds of your time. Expecting new contributors to email the dev list every time they want to request a review of their PR seems far more problematic in term of attracting and retaining new contributors.
On 10/28/20, 8:10 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
-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" <ud...@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
Re: PR process and etiquette
Posted by Owen Nichols <on...@vmware.com>.
Hey Bruce, please consider that non-committers are not permitted to add reviewers themselves, so a consistent convention to indicate when a PR has moved from work-in-progress to ready-for-review will help alert the community when to assign reviewers.
Currently, I see countless creative variations on people adding "WIP" or "DO NOT REVIEW" or other text somewhere in the summary or description. I think the suggestion here is to standardize on using Draft status as the canonical way to communicate this.
Also worth noting, you can change a PR back to 'draft' mode at any time (so whether you forgot initially, or a test failure or review feedback made you realize more work is needed, you can always amend the draft status to communicate whether you're still working, or ready for review again).
I'm not sure why you think this is going to make it more difficult to attract and retain new contributors. The point is to make it easier for new contributors to get their PRs reviewed, and give reviewers more confident that their time is appreciated.
I see your point that as a committer, you have no need to communicate this status, since you can simply add reviewers yourself when you're ready. Asking everyone to use draft status is not the ideal workaround for the GitHub flaw that non-committers can't request reviewers, but it also takes only 2 seconds of your time. Expecting new contributors to email the dev list every time they want to request a review of their PR seems far more problematic in term of attracting and retaining new contributors.
On 10/28/20, 8:10 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
-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" <ud...@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
Re: PR process and etiquette
Posted by Anthony Baker <ba...@vmware.com>.
I think exploring these additions to PR reviews would be pretty helpful. It’s worth spending the extra time to get a PR right before merging.
Anthony
> On Oct 28, 2020, at 8:40 AM, Robert Houghton <rh...@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://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features#git.asf.yamlfeatures-BranchProtection>
>
Re: PR process and etiquette
Posted by Alexander Murmann <am...@vmware.com>.
Let's try to move this towards action:
It seems like we have consensus that a CONTRIBUTING.md file is a great idea.
We also have consensus to recommend in there that one should be mindful to create PRs as drafts if you don't consider them ready for review.
It seems reasonable that one might want a review of incomplete code. I'd recommend calling out in the PR description that the PR is incomplete, what's missing and maybe what areas you'd like reviewers to focus on, given the incompleteness.
I'd love to see some of the things Donal called out around commit messages and PR descriptions included in CONTRIBUTING.md as well.
Is the above an accurate description of this discussion's consensus?
Udo, if we all agree on the above, would you like to take a stab at writing the CONTRIBUTING.md, since you kicked this off? If not, I am happy to give it a shot as well.
________________________________
From: Darrel Schneider <da...@vmware.com>
Sent: Thursday, October 29, 2020 14:33
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for adding a CONTRIBUTING.MD file
________________________________
From: Sarah Abbey <sa...@vmware.com>
Sent: Thursday, October 29, 2020 2:07 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
Regarding knowing who to tag in a PR, because I am working on a very specific aspect of Geode, it was frustrating before I was a committer to not be able to add reviewers since I knew the handful of people that had enough context to review most of the PRs I submitted. But it is also not hard for me to ask these people directly since I work with them every day. I can imagine it would be very frustrating to know exactly who to request a review from, but not be able to add them and not be able to directly contact them or get their attention. It would also be nice to be able to request reviews from people who are not committers, which we may not be able to change due to GitHub limitations. Some of my team members are not committers yet, but I still value their input/review of the code even if their review would not count as an official approving review. Or if we submit a PR that solves an issue raised by a non-committer, it would be nice to have them review it (if they want to/it makes sense for them to) to be sure the issue is addressed. Robert has mentioned that a workaround is to tag those users in a comment.
Since I only feel like I have significant context in one area of Geode, I usually scan the list of PRs for that area unless I'm explicitly tagged as a reviewer. Sometimes, I read other PRs to gain knowledge or if I'm curious, but I don't usually add a review.
Regarding waiting until all commit checks are green, it depends on the PR for me. I usually glance at a PR once I'm tagged as a reviewer or I see something that needs a review in my area of expertise. If it looks like the PR still needs a lot of work and checks like build did not pass, then I typically wait to review it. If most of the checks have passed, and checks that take a long time to run, like DUnit or Acceptance, haven't completed, I will often review it. If certain checks that sometimes have flaky tests are not passing, like DUnit, and all other checks are green, I'll often look at those failures to see if they are related to the PR at all and check Jira to see if there has been an issue filed for them or not. I'll still review the PR and make a comment about the flaky test. If the failure seems related to the PR, I might still review it to see what might've caused the failure.
Whatever we do, our guide to contributing<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FHow%2Bto%2BContribute&data=04%7C01%7Camurmann%40vmware.com%7Ce55b7cc49fcb46cb0faf08d87c525d04%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637396040480339601%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=clpboSHFvmJF6XcMbup9bKnJGXtkj2cbIkX6RHE03Vk%3D&reserved=0> could definitely use an update. We might even consider putting a CONTRIBUTING.MD file directly in our GitHub repo. I've found these guides useful when contributing to other open source projects. I also like contributing to open source projects when my PR is reviewed timely (within a week, though I'm sure we all have different definitions of timely) and any feedback or discussion is constructive and kind.
Sarah
________________________________
From: Donal Evans <do...@vmware.com>
Sent: Thursday, October 29, 2020 3:41 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
As a (relatively) new committer, one of the more difficult aspects of the PR process is knowing who to tag as a reviewer. The last person to touch a class may not actually have the context or depth of knowledge needed for a thorough review if, say, their changes were just refactoring or code cleanup, and if you don't have the luxury of working directly with other committers, it's not always clear who is the "right" person to ask for review. Add to that the fear of overburdening the more knowledgeable committers with endless requests for review, and you can find yourself in a position where the reviews you do end up getting are somewhat perfunctory or only address surface-level issues rather than potential more serious and fundamental problems.
The reverse of this is also something I have struggled with as one of the newer and less knowledgeable members of the community, as I'll sometimes see a PR sat waiting for review that changes areas of the code that I don't know much about and, wanting to help out, make some comments or requests for changes related to things like test naming, code style or other mostly cosmetic issues. Once those have been addressed, I can approve the PR, but I know that I haven't really reviewed it to the standard necessary to have confidence that it's not going to break something. On the one hand, I want to be active and help ensure the quality of code contributions in any way that I can, but on the other, I don't want my approval of a set of changes based on my limited understanding to be taken as a solid confirmation that there are no problems with them.
In terms of things that make the PR process easier as a reviewer, marking PRs as drafts until they're ready for comprehensive review is good, but I also have no problem with offering comments on a draft PR if they relate to things that are unlikely to change, like method or variable names, or even the broad approach being taken, so I don't view the draft status as a barrier to review. I also find it very helpful when the PR description gives an overview of what the PR is doing, not just repeating what's in the JIRA ticket associated with it, since the description of a bug and the description of its fix are often very different. Along a similar vein, descriptive commit messages are valuable since they can help provide context or motivation for why changes were made. The more I understand the contents of a PR in terms of what is being changed, why, and what the (intended) consequences are, the more confident I feel in being able to provide a thorough review.
Donal
________________________________
From: Udo Kohlmeyer <ud...@vmware.com>
Sent: Wednesday, October 28, 2020 5:50 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
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 <db...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <de...@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 <rh...@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%7Camurmann%40vmware.com%7Ce55b7cc49fcb46cb0faf08d87c525d04%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637396040480339601%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=u1Cn1WXvqkQONC81owXad82Xms8zRjW6D6ZCPZZuC14%3D&reserved=0
> >
>
>
>
> From: Bruce Schuchardt <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Darrel Schneider <da...@vmware.com>.
+1 for adding a CONTRIBUTING.MD file
________________________________
From: Sarah Abbey <sa...@vmware.com>
Sent: Thursday, October 29, 2020 2:07 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
Regarding knowing who to tag in a PR, because I am working on a very specific aspect of Geode, it was frustrating before I was a committer to not be able to add reviewers since I knew the handful of people that had enough context to review most of the PRs I submitted. But it is also not hard for me to ask these people directly since I work with them every day. I can imagine it would be very frustrating to know exactly who to request a review from, but not be able to add them and not be able to directly contact them or get their attention. It would also be nice to be able to request reviews from people who are not committers, which we may not be able to change due to GitHub limitations. Some of my team members are not committers yet, but I still value their input/review of the code even if their review would not count as an official approving review. Or if we submit a PR that solves an issue raised by a non-committer, it would be nice to have them review it (if they want to/it makes sense for them to) to be sure the issue is addressed. Robert has mentioned that a workaround is to tag those users in a comment.
Since I only feel like I have significant context in one area of Geode, I usually scan the list of PRs for that area unless I'm explicitly tagged as a reviewer. Sometimes, I read other PRs to gain knowledge or if I'm curious, but I don't usually add a review.
Regarding waiting until all commit checks are green, it depends on the PR for me. I usually glance at a PR once I'm tagged as a reviewer or I see something that needs a review in my area of expertise. If it looks like the PR still needs a lot of work and checks like build did not pass, then I typically wait to review it. If most of the checks have passed, and checks that take a long time to run, like DUnit or Acceptance, haven't completed, I will often review it. If certain checks that sometimes have flaky tests are not passing, like DUnit, and all other checks are green, I'll often look at those failures to see if they are related to the PR at all and check Jira to see if there has been an issue filed for them or not. I'll still review the PR and make a comment about the flaky test. If the failure seems related to the PR, I might still review it to see what might've caused the failure.
Whatever we do, our guide to contributing<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FHow%2Bto%2BContribute&data=04%7C01%7Cdarrel%40vmware.com%7Cfc91b715afda4c923b7b08d87c4ea402%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637396024495116406%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=HRWpheYBXvlF2DamNOtkI%2FcFE3tHrUiVauC6WJXVH9I%3D&reserved=0> could definitely use an update. We might even consider putting a CONTRIBUTING.MD file directly in our GitHub repo. I've found these guides useful when contributing to other open source projects. I also like contributing to open source projects when my PR is reviewed timely (within a week, though I'm sure we all have different definitions of timely) and any feedback or discussion is constructive and kind.
Sarah
________________________________
From: Donal Evans <do...@vmware.com>
Sent: Thursday, October 29, 2020 3:41 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
As a (relatively) new committer, one of the more difficult aspects of the PR process is knowing who to tag as a reviewer. The last person to touch a class may not actually have the context or depth of knowledge needed for a thorough review if, say, their changes were just refactoring or code cleanup, and if you don't have the luxury of working directly with other committers, it's not always clear who is the "right" person to ask for review. Add to that the fear of overburdening the more knowledgeable committers with endless requests for review, and you can find yourself in a position where the reviews you do end up getting are somewhat perfunctory or only address surface-level issues rather than potential more serious and fundamental problems.
The reverse of this is also something I have struggled with as one of the newer and less knowledgeable members of the community, as I'll sometimes see a PR sat waiting for review that changes areas of the code that I don't know much about and, wanting to help out, make some comments or requests for changes related to things like test naming, code style or other mostly cosmetic issues. Once those have been addressed, I can approve the PR, but I know that I haven't really reviewed it to the standard necessary to have confidence that it's not going to break something. On the one hand, I want to be active and help ensure the quality of code contributions in any way that I can, but on the other, I don't want my approval of a set of changes based on my limited understanding to be taken as a solid confirmation that there are no problems with them.
In terms of things that make the PR process easier as a reviewer, marking PRs as drafts until they're ready for comprehensive review is good, but I also have no problem with offering comments on a draft PR if they relate to things that are unlikely to change, like method or variable names, or even the broad approach being taken, so I don't view the draft status as a barrier to review. I also find it very helpful when the PR description gives an overview of what the PR is doing, not just repeating what's in the JIRA ticket associated with it, since the description of a bug and the description of its fix are often very different. Along a similar vein, descriptive commit messages are valuable since they can help provide context or motivation for why changes were made. The more I understand the contents of a PR in terms of what is being changed, why, and what the (intended) consequences are, the more confident I feel in being able to provide a thorough review.
Donal
________________________________
From: Udo Kohlmeyer <ud...@vmware.com>
Sent: Wednesday, October 28, 2020 5:50 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
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 <db...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <de...@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 <rh...@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%7Cdarrel%40vmware.com%7Cfc91b715afda4c923b7b08d87c4ea402%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637396024495116406%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3ENWEamCrjYM0v0i7wthlUSlBNx0T%2FmiG3U9NzKV5Ps%3D&reserved=0
> >
>
>
>
> From: Bruce Schuchardt <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Sarah Abbey <sa...@vmware.com>.
Regarding knowing who to tag in a PR, because I am working on a very specific aspect of Geode, it was frustrating before I was a committer to not be able to add reviewers since I knew the handful of people that had enough context to review most of the PRs I submitted. But it is also not hard for me to ask these people directly since I work with them every day. I can imagine it would be very frustrating to know exactly who to request a review from, but not be able to add them and not be able to directly contact them or get their attention. It would also be nice to be able to request reviews from people who are not committers, which we may not be able to change due to GitHub limitations. Some of my team members are not committers yet, but I still value their input/review of the code even if their review would not count as an official approving review. Or if we submit a PR that solves an issue raised by a non-committer, it would be nice to have them review it (if they want to/it makes sense for them to) to be sure the issue is addressed. Robert has mentioned that a workaround is to tag those users in a comment.
Since I only feel like I have significant context in one area of Geode, I usually scan the list of PRs for that area unless I'm explicitly tagged as a reviewer. Sometimes, I read other PRs to gain knowledge or if I'm curious, but I don't usually add a review.
Regarding waiting until all commit checks are green, it depends on the PR for me. I usually glance at a PR once I'm tagged as a reviewer or I see something that needs a review in my area of expertise. If it looks like the PR still needs a lot of work and checks like build did not pass, then I typically wait to review it. If most of the checks have passed, and checks that take a long time to run, like DUnit or Acceptance, haven't completed, I will often review it. If certain checks that sometimes have flaky tests are not passing, like DUnit, and all other checks are green, I'll often look at those failures to see if they are related to the PR at all and check Jira to see if there has been an issue filed for them or not. I'll still review the PR and make a comment about the flaky test. If the failure seems related to the PR, I might still review it to see what might've caused the failure.
Whatever we do, our guide to contributing<https://cwiki.apache.org/confluence/display/GEODE/How+to+Contribute> could definitely use an update. We might even consider putting a CONTRIBUTING.MD file directly in our GitHub repo. I've found these guides useful when contributing to other open source projects. I also like contributing to open source projects when my PR is reviewed timely (within a week, though I'm sure we all have different definitions of timely) and any feedback or discussion is constructive and kind.
Sarah
________________________________
From: Donal Evans <do...@vmware.com>
Sent: Thursday, October 29, 2020 3:41 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
As a (relatively) new committer, one of the more difficult aspects of the PR process is knowing who to tag as a reviewer. The last person to touch a class may not actually have the context or depth of knowledge needed for a thorough review if, say, their changes were just refactoring or code cleanup, and if you don't have the luxury of working directly with other committers, it's not always clear who is the "right" person to ask for review. Add to that the fear of overburdening the more knowledgeable committers with endless requests for review, and you can find yourself in a position where the reviews you do end up getting are somewhat perfunctory or only address surface-level issues rather than potential more serious and fundamental problems.
The reverse of this is also something I have struggled with as one of the newer and less knowledgeable members of the community, as I'll sometimes see a PR sat waiting for review that changes areas of the code that I don't know much about and, wanting to help out, make some comments or requests for changes related to things like test naming, code style or other mostly cosmetic issues. Once those have been addressed, I can approve the PR, but I know that I haven't really reviewed it to the standard necessary to have confidence that it's not going to break something. On the one hand, I want to be active and help ensure the quality of code contributions in any way that I can, but on the other, I don't want my approval of a set of changes based on my limited understanding to be taken as a solid confirmation that there are no problems with them.
In terms of things that make the PR process easier as a reviewer, marking PRs as drafts until they're ready for comprehensive review is good, but I also have no problem with offering comments on a draft PR if they relate to things that are unlikely to change, like method or variable names, or even the broad approach being taken, so I don't view the draft status as a barrier to review. I also find it very helpful when the PR description gives an overview of what the PR is doing, not just repeating what's in the JIRA ticket associated with it, since the description of a bug and the description of its fix are often very different. Along a similar vein, descriptive commit messages are valuable since they can help provide context or motivation for why changes were made. The more I understand the contents of a PR in terms of what is being changed, why, and what the (intended) consequences are, the more confident I feel in being able to provide a thorough review.
Donal
________________________________
From: Udo Kohlmeyer <ud...@vmware.com>
Sent: Wednesday, October 28, 2020 5:50 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
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 <db...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <de...@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 <rh...@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%7Csabbey%40vmware.com%7C97265ab9a0ce4b7d409308d87c42b20b%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637395973184877711%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eGAKIvashsD8sEE11UigS1p4CVGr4SlvIywCewD0HOM%3D&reserved=0
> >
>
>
>
> From: Bruce Schuchardt <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Donal Evans <do...@vmware.com>.
As a (relatively) new committer, one of the more difficult aspects of the PR process is knowing who to tag as a reviewer. The last person to touch a class may not actually have the context or depth of knowledge needed for a thorough review if, say, their changes were just refactoring or code cleanup, and if you don't have the luxury of working directly with other committers, it's not always clear who is the "right" person to ask for review. Add to that the fear of overburdening the more knowledgeable committers with endless requests for review, and you can find yourself in a position where the reviews you do end up getting are somewhat perfunctory or only address surface-level issues rather than potential more serious and fundamental problems.
The reverse of this is also something I have struggled with as one of the newer and less knowledgeable members of the community, as I'll sometimes see a PR sat waiting for review that changes areas of the code that I don't know much about and, wanting to help out, make some comments or requests for changes related to things like test naming, code style or other mostly cosmetic issues. Once those have been addressed, I can approve the PR, but I know that I haven't really reviewed it to the standard necessary to have confidence that it's not going to break something. On the one hand, I want to be active and help ensure the quality of code contributions in any way that I can, but on the other, I don't want my approval of a set of changes based on my limited understanding to be taken as a solid confirmation that there are no problems with them.
In terms of things that make the PR process easier as a reviewer, marking PRs as drafts until they're ready for comprehensive review is good, but I also have no problem with offering comments on a draft PR if they relate to things that are unlikely to change, like method or variable names, or even the broad approach being taken, so I don't view the draft status as a barrier to review. I also find it very helpful when the PR description gives an overview of what the PR is doing, not just repeating what's in the JIRA ticket associated with it, since the description of a bug and the description of its fix are often very different. Along a similar vein, descriptive commit messages are valuable since they can help provide context or motivation for why changes were made. The more I understand the contents of a PR in terms of what is being changed, why, and what the (intended) consequences are, the more confident I feel in being able to provide a thorough review.
Donal
________________________________
From: Udo Kohlmeyer <ud...@vmware.com>
Sent: Wednesday, October 28, 2020 5:50 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
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 <db...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <de...@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 <rh...@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%7Cdoevans%40vmware.com%7C462363e68fd34ee5050508d87ba4a5dd%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637395294375799885%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N0D%2B%2BEH6aU2Zd3JyxopazrvtF2au9Ze2PwNBEWqWSLs%3D&reserved=0
> >
>
>
>
> From: Bruce Schuchardt <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Bruce Schuchardt <br...@vmware.com>.
I totally agree that folks should use Draft mode for things that aren't ready for review. Beyond that it's easy enough to look at a PR and see if it's in Draft state or hasn't passed checks before putting in an effort to review the code. However, going into the code of a Draft PR or one that hasn't passed the checks can be useful in that you can give early feedback if you see problems.
For the partial-signoff problem I've always just left a comment that the portion of the PR that I feel competent to review is ready to merge. I don't give the PR an approval in that case.
On 10/28/20, 5:50 PM, "Udo Kohlmeyer" <ud...@vmware.com> wrote:
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 <db...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <de...@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 <rh...@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%7Cbruces%40vmware.com%7Ccdd49457cfd54990524108d87ba4a89c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637395294432731053%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=jSkrBwqgjJ83QGTeEsB5NJkOU9j4zq2THWUEDPPTee0%3D&reserved=0
> >
>
>
>
> From: Bruce Schuchardt <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Alberto Gomez <al...@est.tech>.
Hi there,
Here come my 2 cents.
@Udo Kohlmeyer<ma...@vmware.com>, thanks for your proposals to make this community better, and also for your willingness to get feedback from people who are new to the community.
In my experience, one of the tricky parts working in the community is getting reviewers for PRs. It is a bit of a mystery what will happen once you submit a PR. Sometimes you get a review in a few hours. Sometimes you do not and ask in the list for reviewers and after that, sometimes you get reviewers soon, and sometimes you don't, and you need to insist in the list. I have sometimes asked for a review to a particular person via e-mail as I do not have permissions to assign reviewers to PRs and sometimes have not received any answer.
I figure the response time is very dependent on how busy people. Anyhow, it is the uncertainty of what is going on behind the scenes what makes things hard.
If any proposal makes the review process more predictable, I am up for it. I think Udo's reflections to come up with a consistent approach to the review process are very valuable.
In my opinion, it is a good idea to submit draft PRs while we do not have the green light from the CI. I have many times submitted PRs, gone to sleep just to realize the morning after that some test cases in the CI failed (either due to flaky test cases or due to my changes). Sometimes I had already gotten a review and I would have preferred to have it once the CI was clear. Other times I did not get a review and I wondered if that (or those) failures would keep reviewers away from my PR given that they once looked at it and had test cases failures.
Alberto G.
________________________________
From: Udo Kohlmeyer <ud...@vmware.com>
Sent: Thursday, October 29, 2020 1:50 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
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 <db...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <de...@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 <rh...@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 <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Udo Kohlmeyer <ud...@vmware.com>.
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 <db...@apache.org>
Date: Thursday, October 29, 2020 at 4:20 AM
To: dev@geode.apache.org <de...@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 <rh...@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 <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Dave Barnes <db...@apache.org>.
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 <rh...@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://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features#git.asf.yamlfeatures-BranchProtection
> >
>
>
>
> From: Bruce Schuchardt <br...@vmware.com>
> Date: Wednesday, October 28, 2020 at 8:10 AM
> To: dev@geode.apache.org <de...@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" <ud...@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
>
>
Re: PR process and etiquette
Posted by Robert Houghton <rh...@vmware.com>.
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://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features#git.asf.yamlfeatures-BranchProtection>
From: Bruce Schuchardt <br...@vmware.com>
Date: Wednesday, October 28, 2020 at 8:10 AM
To: dev@geode.apache.org <de...@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" <ud...@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
Re: PR process and etiquette
Posted by Bruce Schuchardt <br...@vmware.com>.
-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" <ud...@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
Re: PR process and etiquette
Posted by Udo Kohlmeyer <ud...@vmware.com>.
Great ideas Owen.
I do apologize for the BIG lump of text… stupid formatting of lack thereof…
--Udo
From: Owen Nichols <on...@vmware.com>
Date: Wednesday, October 28, 2020 at 12:03 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for using GitHub's draft status to indicate work-in-progress.
Many great suggestions here, however I generally prefer that we don't squash commits at any point except the final Squash and Merge to develop. I find it insightful to see how the work evolved. I also find that review comments may start coming in even before you are "ready" for review, and a squash or force-push "loses" those comments.
One thing I would like to see more of is PR summaries that explain *why* the change is being made, not just *what* is being changed.
Thanks Udo for looking for ways to make the community process work even better!
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <ud...@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 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
Re: PR process and etiquette
Posted by Blake Bender <bb...@vmware.com>.
Oops, sorry for the confusion! I’ve been working through Mario’s PRs a lot lately.
From: Alberto Gomez <al...@est.tech>
Date: Wednesday, October 28, 2020 at 7:10 AM
To: "dev@geode.apache.org" <de...@geode.apache.org>, Blake Bender <bb...@vmware.com>
Subject: Re: PR process and etiquette
+1 to draft PRs.
By the way @Blake Bender<ma...@vmware.com>, it's me the one having the draft PR for GEODE-8318.
Alberto G.
________________________________
From: Blake Bender <bb...@vmware.com>
Sent: Wednesday, October 28, 2020 2:28 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for draft PRs. Native has been using these for a few months now, and they're quite effective. Right now, for example, we have 6 PRs up, 3 of which are draft. They also turn out to be a convenient way to share work, in certain circumstances. Mario, for instance, has a draft up for GEODE-8318 that is strictly WIP. By having it up as a draft PR, I get notifications when changes are pushed, and can run internal tooling and let him know if I find issues.
Thanks,
Blake
On 10/28/20, 1:03 AM, "Udo Kohlmeyer" <ud...@vmware.com> wrote:
Great information Darrel. Thank you for sharing that.
--Udo
From: Darrel Schneider <da...@vmware.com>
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to be aware of is that if your pr branch has conflicts and it is in draft mode then your pr tests will not run and the pr page will not tell you that conflicts exist. If you see that the pr tests are not actually running and it is in draft mode then try merging develop to your pr branch and resolve the conflicts.
________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for using GitHub's draft status to indicate work-in-progress.
Many great suggestions here, however I generally prefer that we don't squash commits at any point except the final Squash and Merge to develop. I find it insightful to see how the work evolved. I also find that review comments may start coming in even before you are "ready" for review, and a squash or force-push "loses" those comments.
One thing I would like to see more of is PR summaries that explain *why* the change is being made, not just *what* is being changed.
Thanks Udo for looking for ways to make the community process work even better!
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <ud...@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 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
Re: PR process and etiquette
Posted by Alberto Gomez <al...@est.tech>.
+1 to draft PRs.
By the way @Blake Bender<ma...@vmware.com>, it's me the one having the draft PR for GEODE-8318.
Alberto G.
________________________________
From: Blake Bender <bb...@vmware.com>
Sent: Wednesday, October 28, 2020 2:28 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for draft PRs. Native has been using these for a few months now, and they're quite effective. Right now, for example, we have 6 PRs up, 3 of which are draft. They also turn out to be a convenient way to share work, in certain circumstances. Mario, for instance, has a draft up for GEODE-8318 that is strictly WIP. By having it up as a draft PR, I get notifications when changes are pushed, and can run internal tooling and let him know if I find issues.
Thanks,
Blake
On 10/28/20, 1:03 AM, "Udo Kohlmeyer" <ud...@vmware.com> wrote:
Great information Darrel. Thank you for sharing that.
--Udo
From: Darrel Schneider <da...@vmware.com>
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to be aware of is that if your pr branch has conflicts and it is in draft mode then your pr tests will not run and the pr page will not tell you that conflicts exist. If you see that the pr tests are not actually running and it is in draft mode then try merging develop to your pr branch and resolve the conflicts.
________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for using GitHub's draft status to indicate work-in-progress.
Many great suggestions here, however I generally prefer that we don't squash commits at any point except the final Squash and Merge to develop. I find it insightful to see how the work evolved. I also find that review comments may start coming in even before you are "ready" for review, and a squash or force-push "loses" those comments.
One thing I would like to see more of is PR summaries that explain *why* the change is being made, not just *what* is being changed.
Thanks Udo for looking for ways to make the community process work even better!
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <ud...@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 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
Re: PR process and etiquette
Posted by Blake Bender <bb...@vmware.com>.
+1 for draft PRs. Native has been using these for a few months now, and they're quite effective. Right now, for example, we have 6 PRs up, 3 of which are draft. They also turn out to be a convenient way to share work, in certain circumstances. Mario, for instance, has a draft up for GEODE-8318 that is strictly WIP. By having it up as a draft PR, I get notifications when changes are pushed, and can run internal tooling and let him know if I find issues.
Thanks,
Blake
On 10/28/20, 1:03 AM, "Udo Kohlmeyer" <ud...@vmware.com> wrote:
Great information Darrel. Thank you for sharing that.
--Udo
From: Darrel Schneider <da...@vmware.com>
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to be aware of is that if your pr branch has conflicts and it is in draft mode then your pr tests will not run and the pr page will not tell you that conflicts exist. If you see that the pr tests are not actually running and it is in draft mode then try merging develop to your pr branch and resolve the conflicts.
________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for using GitHub's draft status to indicate work-in-progress.
Many great suggestions here, however I generally prefer that we don't squash commits at any point except the final Squash and Merge to develop. I find it insightful to see how the work evolved. I also find that review comments may start coming in even before you are "ready" for review, and a squash or force-push "loses" those comments.
One thing I would like to see more of is PR summaries that explain *why* the change is being made, not just *what* is being changed.
Thanks Udo for looking for ways to make the community process work even better!
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <ud...@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 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
Re: PR process and etiquette
Posted by Udo Kohlmeyer <ud...@vmware.com>.
Great information Darrel. Thank you for sharing that.
--Udo
From: Darrel Schneider <da...@vmware.com>
Date: Wednesday, October 28, 2020 at 3:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 to your idea of using "draft" mode until things are green. Something to be aware of is that if your pr branch has conflicts and it is in draft mode then your pr tests will not run and the pr page will not tell you that conflicts exist. If you see that the pr tests are not actually running and it is in draft mode then try merging develop to your pr branch and resolve the conflicts.
________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for using GitHub's draft status to indicate work-in-progress.
Many great suggestions here, however I generally prefer that we don't squash commits at any point except the final Squash and Merge to develop. I find it insightful to see how the work evolved. I also find that review comments may start coming in even before you are "ready" for review, and a squash or force-push "loses" those comments.
One thing I would like to see more of is PR summaries that explain *why* the change is being made, not just *what* is being changed.
Thanks Udo for looking for ways to make the community process work even better!
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <ud...@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 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
Re: PR process and etiquette
Posted by Darrel Schneider <da...@vmware.com>.
+1 to your idea of using "draft" mode until things are green. Something to be aware of is that if your pr branch has conflicts and it is in draft mode then your pr tests will not run and the pr page will not tell you that conflicts exist. If you see that the pr tests are not actually running and it is in draft mode then try merging develop to your pr branch and resolve the conflicts.
________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Tuesday, October 27, 2020 6:03 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PR process and etiquette
+1 for using GitHub's draft status to indicate work-in-progress.
Many great suggestions here, however I generally prefer that we don't squash commits at any point except the final Squash and Merge to develop. I find it insightful to see how the work evolved. I also find that review comments may start coming in even before you are "ready" for review, and a squash or force-push "loses" those comments.
One thing I would like to see more of is PR summaries that explain *why* the change is being made, not just *what* is being changed.
Thanks Udo for looking for ways to make the community process work even better!
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <ud...@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 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
Re: PR process and etiquette
Posted by Owen Nichols <on...@vmware.com>.
+1 for using GitHub's draft status to indicate work-in-progress.
Many great suggestions here, however I generally prefer that we don't squash commits at any point except the final Squash and Merge to develop. I find it insightful to see how the work evolved. I also find that review comments may start coming in even before you are "ready" for review, and a squash or force-push "loses" those comments.
One thing I would like to see more of is PR summaries that explain *why* the change is being made, not just *what* is being changed.
Thanks Udo for looking for ways to make the community process work even better!
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <ud...@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 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