MNT: prevent merging using labels + branch protection rules

[story645]

PR summary

Kinda been bugging @QuLogic about this for a while...

This PR + some one w. org access adding a branch protection rule that blocks merges when this status check fails would allow us to grey out merging using a specific set of labels.

As you can see in the workflow, the status check fails b/c this is labeled "status: needs comment/discussion" (and I tried to turn off most of the CI checks so folks can play with adding and removing the label)

The motivation is b/c 'request changes' for reviews can be used in two ways:

1. needs change but will be merged after change is implemented
2. needs meta discussion, may not be merged

And I think using it for 2. has made folks wary of using it for 1, and instead some maintainers use "leave comment" to communicate what they would need to see changed before they merge.

My hope is that decoupling small comments from requesting technical changes from blocking a PR b/c there isn't consensus (or on some other label b/c it's an or list) can help us better communicate and manage expectations around what needs to happen next in the PR - make small set of changes or wait on discussion - and make maintainers more likely to use 'request changes' instead of 'comment' to communicate what they're waiting on being implemented before approving. If a maintainer disagrees on a "requested change", then they can always add a blocking label.

Bonus is it also is a way to flag "this is not in draft, but also not ready for merging" for POC type work.

PR checklist

• "closes #0000" is in the body of the PR description to link the related issue
• new and changed code is tested
• Plotting related features are demonstrated in an example
• New Features and API Changes are noted with a directive and release note
• Documentation complies with general and docstring guidelines

[timhoffm]

I fundamentally support the idea of distinguishing the two cases better.

I'm always hesitant with request changes because somebody has to dismiss the review afterwards, and GitHub makes quite a song and dance out of that, so that it feels awkward to dismiss someone's else review,
But technically the process is right. We should encourage that any reviewer can and should dismiss if they see that the comments are properly addressed.

[QuLogic]

Note, I couldn't add a rule on this workflow passing yet, probably because it's not had any runs in this repo. So we can't really preview it (at least with this workflow).

[story645]

So we can't really preview it (at least with this workflow).

Have I managed a catch 22 where we can't see how it fully works before discussing b/c that requires a merge but it'd be nice to see how it fully works before merging? 😶

[QuLogic]

I could set it on other checks, but that would likely affect other PRs. However, I don't think it would look too different from how it does now with the "Merging is blocked" error due to not having any approving reviews.

[story645]

However, I don't think it would look too different from how it does now with the "Merging is blocked" error due to not having any approving reviews.

Yeah, in all seriousness I figure it'll work like the required label check on any sphinx gallery PR -> which👀 how they label the required ones

[tacaswell]

This seems reasonable.

[story645]

@ksunden has a pass and a fail state now

[QuLogic]

Hmm, I thought we had merged this already. It's not really waiting for another PR, right? That was just for testing?

[story645]

It's not really waiting for another PR, right? That was just for testing?

Yup!

[story645]

Though now it doesn't seem to be working 🤦‍♀️

[QuLogic]

It looks green to me?

[story645]

This is what I see? Maybe it's cause I'm on the app?

[QuLogic]

Here I see it passing (the 1 failing is codecov):

But confusingly, if I go to the Checks tab, then there are four copies of this check, and two of them failed. But the three old ones are from Feb 8, passing is from 2 days ago.

[story645]

ok, so looks like it's working but there's just some lag sometimes

[jklymak]

I'm not sold on the utility of this, and it just seems to needlessly complicate the status of PRs. We already have "block" and "Draft" as tools to indicate changes need to be made, or we can even go so far as to close PRs. We can also communicate in the comments. With this change we have to further remember a list of tags that will opaquely block merging and remember which ones we need to remove to un-block. That doesn't seem that helpful to our workflows.

[story645]

With this change we have to further remember a list of tags that will opaquely block merging and remember which ones we need to remove to un-block

It's the two tags that already informally mean "don't merge" without doing the further action described in the tag . Basically all I'm doing here is programmatically enforcing an unwritten convention we already have.

[jklymak]

I've generally used those tags in the past to mean that something needs input from other people, and/or discussion at the weekly meeting. I'm not sure I've ever used them to mean "don't merge this".

[story645]

I've generally used those tags in the past to mean that something needs input from other people, and/or discussion at the weekly meeting. I'm not sure I've ever used them to mean "don't merge this".

I use it to mean "needs input/discussion" because this does not have consensus and therefore it should not be merged. Per our definition

I only ever use it to mean "hey, don't merge this" (and has always been how I interpret it) and generally just tag people if I think something needs more general input.

I only resort to using "request changes" as a block when I feel the label is getting ignored (which per this conversation is apparently b/c we have very mismatched expectations), b/c as I said up top, I fundamentally think we should be using different mechanisms to communicate:

1. needs change but will be merged after change is implemented
2. needs meta discussion, may not be merged

One of my hopes here is that making the mappings clearer will make it easier for folks to dismiss changes or remove labels when the issue is addressed.

[QuLogic]

We've already discussed this on the call and decided to try it out, and see if it's effective.

[QuLogic]

This is now required on PRs. I couldn't find it as the workflow name, but it turns out it's listed by job name.