Auto labeling PRs based on what stage they're in
I don't know about the rest of you, but I want an easy way to glance at a pull request and know what it's currently blocking it from being merged (if I'm wrong then let me know). I opened https://github.com/python/core-workflow/issues/94 for this idea, but I figured what the exact stages should be deserves a wider discussion. The stages I see are: 1. *Awaiting first review*: no one has reviewed the PR 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) 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) 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) 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?) Am I missing a step?
On 18 June 2017 at 05:07, Brett Cannon <brett@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", and have it advance only for a positive review with no negative reviews.
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.
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)
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.
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. 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. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Sat, 17 Jun 2017 at 18:11 Nick Coghlan <ncoghlan@gmail.com> wrote:
+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).
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).
Guido squashed the title prefix idea at the language summit (it's what I originally proposed, so this label approach was the compromise).
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).
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).
Donald beat you to the suggestion :) https://github.com/python/core-workflow/issues/29
On 19 June 2017 at 09:02, Brett Cannon <brett@python.org> wrote:
Sorry, yeah, that was a complete tangent. I was mainly trying to explain why I think we genuinely need a core developer's review before promoting a PR that far in the process. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 19 June 2017 at 02:36, Brett Cannon <brett@python.org> wrote:
I'm also fine with having mixed reviews mean "it's a core dev's problem now".
2 humans - the original submitter, and whoever did the initial review (if they weren't a core dev).
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).
Ah, fair enough. In that case, I agree that a "Bedevere: ready for review" comment to request removal of the label make sense.
OK, that rationale makes sense. Given that, I'd suggest making the three review states: - awaiting review - awaiting core review - awaiting change review
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@gmail.com | Brisbane, Australia
OK, here is a dot graph that lays out the stages and the flow between them based on what has been discussed so far. You can use http://www.webgraphviz.com/ to view the graph in a browser. Anything green requires/blocks on a core dev, blue requires/blocks on the PR creator, and orange is open to anyone. digraph "PR stages" { "New PR" [color=blue] "Awaiting review" [shape=box, color=orange] "Awaiting core review" [shape=box, color=green] "Awaiting changes" [shape=box, color=blue] "Awaiting change review" [shape=box, color=green] "Awaiting merge" [shape=box, color=green] "New PR" -> "Awaiting review" [label="PR created by non-core dev", color=blue] "Awaiting review" -> "Awaiting core review" [label="Non-core review", color=orange] "Awaiting core review" -> "Awaiting changes" [label="Core dev requests changes", color=green] "Awaiting changes" -> "Awaiting change review" [label="PR creator addresses changes", color=blue] "Awaiting change review" -> "Awaiting changes" [label="Core dev requests changes", color=green] "Awaiting change review" -> "Awaiting merge" [label="Core dev approves PR", color=green] "Awaiting review" -> "Awaiting merge" [label="Core dev approves PR", color=green] "Awaiting review" -> "Awaiting changes" [label="Core dev requests changes", color=green] "Awaiting core review" -> "Awaiting merge" [label="Core dev approves PR", color=green] "New PR" -> "Awaiting merge" [label="PR by core dev", color=green] On Mon, 19 Jun 2017 at 05:08 Nick Coghlan <ncoghlan@gmail.com> wrote:
On 20 June 2017 at 03:39, Brett Cannon <brett@python.org> wrote:
This looks good to me. I also think there are a few extra exceptional flows, but we can probably say "manipulate the labels directly" for these: "Awaiting changes" -> "Awaiting merge": Core dev implements changes themselves and pushes to submitter's branch Core dev PR -> "Awaiting core review" Core dev PR -> "Awaiting changes" Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Mon, Jun 19, 2017, 18:15 Nick Coghlan, <ncoghlan@gmail.com> wrote:
They could change their review to handle this. themselves and pushes to submitter's branch
Core dev PR -> "Awaiting core review"
Yeah, that could be manual. Core dev PR -> "Awaiting changes"
If this is due to a review from a core dev then it's just like any other core review as you don't want to ignore such a review. -Brett
On 20 June 2017 at 11:55, Brett Cannon <brett@python.org> wrote:
True.
True, but I was thinking of the "WIP" case - GitHub doesn't let you review your own PRs, so you can't give yourself a negative review to indicate that the PR is incomplete. However, setting "Awaiting changes" explicitly will work fine. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
https://github.com/python/bedevere/pull/33 contains the PR I'm planning to merge at some point to handle the automatic labeling of PRs based on their current stage. The diagram is in the waiting.py file as a string literal so you can use webgraphviz.com if you want to look at the stages and how they transition. Unless someone points out a major flaw or speaks up that they think the labels or this idea is really bad, I will probably merge it sometime next week (after I get my test coverage back up). On Sat, 17 Jun 2017 at 12:07 Brett Cannon <brett@python.org> wrote:
On 18 June 2017 at 05:07, Brett Cannon <brett@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", and have it advance only for a positive review with no negative reviews.
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.
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)
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.
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. 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. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Sat, 17 Jun 2017 at 18:11 Nick Coghlan <ncoghlan@gmail.com> wrote:
+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).
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).
Guido squashed the title prefix idea at the language summit (it's what I originally proposed, so this label approach was the compromise).
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).
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).
Donald beat you to the suggestion :) https://github.com/python/core-workflow/issues/29
On 19 June 2017 at 09:02, Brett Cannon <brett@python.org> wrote:
Sorry, yeah, that was a complete tangent. I was mainly trying to explain why I think we genuinely need a core developer's review before promoting a PR that far in the process. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 19 June 2017 at 02:36, Brett Cannon <brett@python.org> wrote:
I'm also fine with having mixed reviews mean "it's a core dev's problem now".
2 humans - the original submitter, and whoever did the initial review (if they weren't a core dev).
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).
Ah, fair enough. In that case, I agree that a "Bedevere: ready for review" comment to request removal of the label make sense.
OK, that rationale makes sense. Given that, I'd suggest making the three review states: - awaiting review - awaiting core review - awaiting change review
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@gmail.com | Brisbane, Australia
OK, here is a dot graph that lays out the stages and the flow between them based on what has been discussed so far. You can use http://www.webgraphviz.com/ to view the graph in a browser. Anything green requires/blocks on a core dev, blue requires/blocks on the PR creator, and orange is open to anyone. digraph "PR stages" { "New PR" [color=blue] "Awaiting review" [shape=box, color=orange] "Awaiting core review" [shape=box, color=green] "Awaiting changes" [shape=box, color=blue] "Awaiting change review" [shape=box, color=green] "Awaiting merge" [shape=box, color=green] "New PR" -> "Awaiting review" [label="PR created by non-core dev", color=blue] "Awaiting review" -> "Awaiting core review" [label="Non-core review", color=orange] "Awaiting core review" -> "Awaiting changes" [label="Core dev requests changes", color=green] "Awaiting changes" -> "Awaiting change review" [label="PR creator addresses changes", color=blue] "Awaiting change review" -> "Awaiting changes" [label="Core dev requests changes", color=green] "Awaiting change review" -> "Awaiting merge" [label="Core dev approves PR", color=green] "Awaiting review" -> "Awaiting merge" [label="Core dev approves PR", color=green] "Awaiting review" -> "Awaiting changes" [label="Core dev requests changes", color=green] "Awaiting core review" -> "Awaiting merge" [label="Core dev approves PR", color=green] "New PR" -> "Awaiting merge" [label="PR by core dev", color=green] On Mon, 19 Jun 2017 at 05:08 Nick Coghlan <ncoghlan@gmail.com> wrote:
On 20 June 2017 at 03:39, Brett Cannon <brett@python.org> wrote:
This looks good to me. I also think there are a few extra exceptional flows, but we can probably say "manipulate the labels directly" for these: "Awaiting changes" -> "Awaiting merge": Core dev implements changes themselves and pushes to submitter's branch Core dev PR -> "Awaiting core review" Core dev PR -> "Awaiting changes" Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Mon, Jun 19, 2017, 18:15 Nick Coghlan, <ncoghlan@gmail.com> wrote:
They could change their review to handle this. themselves and pushes to submitter's branch
Core dev PR -> "Awaiting core review"
Yeah, that could be manual. Core dev PR -> "Awaiting changes"
If this is due to a review from a core dev then it's just like any other core review as you don't want to ignore such a review. -Brett
On 20 June 2017 at 11:55, Brett Cannon <brett@python.org> wrote:
True.
True, but I was thinking of the "WIP" case - GitHub doesn't let you review your own PRs, so you can't give yourself a negative review to indicate that the PR is incomplete. However, setting "Awaiting changes" explicitly will work fine. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
https://github.com/python/bedevere/pull/33 contains the PR I'm planning to merge at some point to handle the automatic labeling of PRs based on their current stage. The diagram is in the waiting.py file as a string literal so you can use webgraphviz.com if you want to look at the stages and how they transition. Unless someone points out a major flaw or speaks up that they think the labels or this idea is really bad, I will probably merge it sometime next week (after I get my test coverage back up). On Sat, 17 Jun 2017 at 12:07 Brett Cannon <brett@python.org> wrote:
participants (3)
-
Brett Cannon -
Mariatta Wijaya -
Nick Coghlan