Proposal: add GitHub PR labels

[wchargin]

@decentralion

So to the extent we are coming up with policy here, do we want a policy that commits are good to merge as soon as they are approved and have no un-approved dependencies? Or do we want to keep giving PR authors responsibility to decide when/how to merge their pulls?

PR authors should always have the responsibility of deciding when and how to merge their pull requests. There are lots of cases when that is not “immediately after it’s approved”. For instance, I might send out a PR for review before going to lunch, but want to make sure to do some extra end-to-end testing when I get back. A quick approval in the middle shouldn’t force it to be merged.

@decentralion

the stock UI doesn’t have any big advantages compared to labels.

The stock UI does have a big advantage compared to labels: the “Review requests” dashboard (https://github.com/pulls/review-requested) shows you all pull requests for which you still need to provide a review:

This is, of course, properly integrated with the “Request re-review” action.

I check this many times per day (usually, multiple times per hour). Someone on my team at work proposed using this workflow about a year ago, and everyone got used to it in no time. There’s not really anything to learn; it works very well.

@decentralion

However, this is also a manual action which we’ll need to train people to do

It’s a manual action, but it kind of has to be. You generally don’t want to set the state to “needs additional review” whenever additional changes are pushed. In most cases, when you’re ready for re-review you’ll also be submitting a counter-review to respond address comments, so you’re already in the UI and the action is top-of-mind.

@decentralion

At a glance, how easy is it to find the build changes in this screenshot? And how easy is it to find which ones are approved and ready?

I’m not sure why you’d want to find all the pull requests that have an approved review, but if you do, just filter for pull requests with approved reviews (equivalently, search for review:approved):

@decentralion

If we agree to use labels for review state, it sounds like we could use GitHub actions to build automation to keep the labels up-to-date (set the label when there’s an approving review, reset the label whenever there’s a push, etc).

We surely could, but that’s automation that we have to write and maintain. Because it’s custom, we’ll also lose any familiarity benefits from people who are used to the normal GitHub UI. We’ll have to deal with awkward ACLing issues—external contributors can’t change labels on PRs (which is by design, because labels are often used to bless a PR to run on a trusted CI system), so how is an external contributor supposed to manually change the merge state if (e.g.) they realize that it’s not actually ready for re-review yet? Do we need to write a bot that responds to commands to change the labels? All this complexity really needs a compelling motivation to be worth its weight, and I’ve yet to be convinced that such a motivation exists.