[core-workflow] Auto labeling PRs based on what stage they're in

[Brett Cannon]

On Sat, 17 Jun 2017 at 18:11 Nick Coghlan <ncoghlan at gmail.com> wrote:

> On 18 June 2017 at 05:07, Brett Cannon <brett at python.org> wrote:
> > The stages I see are:
> >
> > 1. Awaiting first review: no one has reviewed the PR
>
> I'd just make this "Awaiting review",


+1 to the simplification of the name.


> and have it advance only for a
> positive review with no negative reviews.
>

But wouldn't that lead to more than one stage being set since this would
perpetually be set until you were ready to merge? As I said later, I don't
know if we want to gate on a non-core devs approval as it might not be
reasonable (e.g. we have had some non-core devs be extremely picky about
PEP 8 conformance).


>
> > 2. Awaiting core review: a non-core dev has reviewed the PR, but there
> still needs to be that initial core review (still not sure if this is worth
> the distinction or how complex it will make the code, but it may encourage
> people to help in the reviewing process instead of automatically diving
> into coding if their review leads to a visible stage change)
>
> I think this is worth having for the reason you give - anyone can
> advance the state to "awaiting core review", and it gives a visual
> indication of "at least 2 humans think this is a reasonable patch" for
> core developers looking to pick up pre-filtered proposals.
>

Don't you mean "one other person" based on the advancement from "awaiting
review"? Sorry if I wasn't clear before, but what I was thinking was that
if a core dev did the initial review then this stage would be skipped. IOW
a single core dev review is all that's needed to get out of any "I need a
review" stage.

If you do mean you want to require two reviewers for all PRs, I'm not sure
if we are there yet. We already have a shortage of reviewers as it is; at
the language summit I mentioned that we were at a +3 delta for open PRs
each day, so slowing things down by requiring two reviews is premature
until we get that growth rate of open PRs under control (not to say I
wouldn't love to get to the point of requiring two reviews, but I think we
need to get a few more pieces in place before we do that, plus we have not
proven that non-core devs are going to step up to help with reviewing
either).


>
> > 3. Awaiting changes: a review has been done, but changes were requested
> (probably wouldn't reach this stage for non-core reviews; a bit tough to
> block on a non-core review as their asks may not be reasonable and so
> shouldn't automatically signal that what someone that they must do what is
> requested of them in that instance)
>
> +1 for only getting here based on negative core reviews. An
> alternative to requiring a special comment to exit this state would be
> to add a "WIP: " prefix to the PR title when the label is added - that
> way the submitter can just remove the prefix to indicate that they
> have made the requested changes and are ready for another round of
> review, while folks could also add the prefix manually to indicate the
> change *isn't* ready for review. (GitLab has specific handling for
> "WIP:" and "[WIP]" to prevent merges, and it's really handy as a
> communications tool when you want feedback on a draft change, but also
> want to make it completely clear that the change isn't ready to be
> merged yet)
>

Guido squashed the title prefix idea at the language summit (it's what I
originally proposed, so this label approach was the compromise).


>
> > 4. Awaiting Nth review/re-review: the PR submitter believes all the
> requests have been addressed in all reviews and so is waiting on the
> reviewer(s) who requested changes to review again (will need to provide a
> way to leave a message that signals to Bedevere that the submitter feels
> ready for another review)
>
> I don't see a need for this state - going back to "Awaiting core
> review" should suffice.
>

So the reason I made this separate is if you're in the middle of working
with someone on a PR and you're already reviewing it, do I really need to
get involved? If I'm perusing the list of open PRs, wouldn't it be better
for me to look for another PR that has zero reviews than pile on to another
PR that already has a core dev reviewing it?

But if you take a view of "more reviews is always good" then there's no
need to distinguish as you suggested. Then you could use the fact that you
were notified by GitHub as the signal that you put in a review (or if
Bedevere makes a comment notifying the change in status and @ mentions you
which would be easy to filter for in emails).


>
> > 5. Awaiting merge: all reviews approve of the PR (we could also drop the
> "awaiting core review" stage and go straight here even for non-core reviews
> that are all approvals, but maybe that's leaning on non-core devs too much
> and giving false hope to PR submitters?)
>
> I assume we'd mainly get here based on all positive core reviews, no
> pending negative core reviews, but a pending CI run to check
> everything is working as expected.


It can also be reached even if the CI is cleared but the PR will require
backporting and the person who plans to do the merge + backport doesn't
have the time ATM (e.g. I did a review of a PR that applies to master and
3.6 on my lunch break, but I'm waiting until I have time to run
cherry_picker at home to do the merge so I don't lose track of the fact
that I need to do the backport).


> At some point in the future, we
> could potentially even go to an OpenStack style flow where Bedevere
> actually *merges* patches in this state if their CI run succeeds, so
> it wouldn't be reasonable to get here based solely on non-core
> reviews.
>

Donald beat you to the suggestion :)
https://github.com/python/core-workflow/issues/29
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/core-workflow/attachments/20170618/ec44ecdb/attachment.html>