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

[Nick Coghlan]

On 19 June 2017 at 02:36, Brett Cannon <brett at python.org> wrote:
> 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).

I'm also fine with having mixed reviews mean "it's a core dev's problem now".

>> 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"?

2 humans - the original submitter, and whoever did the initial review
(if they weren't a core dev).

> 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.

Right, that's what I was assuming as well.

I also guess that when the submitter *is* a core dev, we'd send a PR
straight to "awaiting merge" unless we had already explicitly added
the "awaiting changes" or "awaiting core review" label.

> If you do mean you want to require two reviewers for all PRs, I'm not sure
> if we are there yet.

No, I didn't mean that (although I can see how you read it that way) -
I still don't think we should even make reviews mandatory for core dev
submitted PRs.

While I do think it would be nice to have the luxury of being able to
afford to implement such a policy, it's the kind of workflow that's
really only feasible when you have multiple folks handling reviews on
a regular basis as part of a full-time job (and even then it can cause
problems in niche areas of a code base where nobody feels particularly
well-equipped to carry out an authoritative review).

>> +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.
>
> Guido squashed the title prefix idea at the language summit (it's what I
> originally proposed, so this label approach was the compromise).

Ah, fair enough. In that case, I agree that a "Bedevere: ready for
review" comment to request removal of the label make sense.

>> > 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?

OK, that rationale makes sense. Given that, I'd suggest making the
three review states:

- awaiting review
- awaiting core review
- awaiting change review

>> > 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).

Aye, I can see that.

FWIW, I was doing things that way for a while, but eventually realised
it made more sense for me to make use of the "backport needed" state
on BPO, since that's closer to our target workflow with automated
backports. It also means I can batch up a bunch of relatively
mechanical backports to do via a local checkout later, rather than
having to switch back and forth between code review and backporting
things.

It definitely isn't perfect (Mariatta ended up cleaning up a few of
the backports from the last batch I did after I'd left them pending
for over a week), but given that the original PRs had previously been
lingering for months, I still count it as an improvement over my
initial approach ;)

Cheers,
Nick.

-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia