Oops, sorry for the confusion! I’ve been working through Mario’s PRs a lot lately.
From: Alberto Gomez <alberto.go...@est.tech> Date: Wednesday, October 28, 2020 at 7:10 AM To: "dev@geode.apache.org" <dev@geode.apache.org>, Blake Bender <bbl...@vmware.com> Subject: Re: PR process and etiquette +1 to draft PRs. By the way @Blake Bender<mailto:bbl...@vmware.com>, it's me the one having the draft PR for GEODE-8318. Alberto G. ________________________________ From: Blake Bender <bbl...@vmware.com> Sent: Wednesday, October 28, 2020 2:28 PM To: dev@geode.apache.org <dev@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" <u...@vmware.com> wrote: Great information Darrel. Thank you for sharing that. --Udo From: Darrel Schneider <dar...@vmware.com> Date: Wednesday, October 28, 2020 at 3:32 PM To: dev@geode.apache.org <dev@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 <onich...@vmware.com> Sent: Tuesday, October 27, 2020 6:03 PM To: dev@geode.apache.org <dev@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" <u...@vmware.com> wrote: Dear Apache Geode Devs, It is really great going through all the PRs that been submitted. As Josh Long is known to say: "I work for PRs". Whilst going through some of the PRs I do see that there are many PRs that have multiple commits against the PR. I know that the PR submission framework kicks off more testing than we do on our own local machines. It is extremely uncommon to submit a PR the first time and have all tests go green. Which 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
