github: added explicit do not merge label to label check #29494
Conversation
|
Aren't draft pull requests enough?
Since I believe we do not care about code owners, effectively draft is equivalent to do not merge. Am I overlooking something? |
Should be, but you also had a very explicit DO NOT MERGE on #29493 on top so I figured since we already had the infrastructure it wasn't gonna cost anything to add it? |
|
I did that explicit DO NOT MERGE because I was not sure whether full CI was running on draft PRs and thus whether I could use them . Since I know now that they are, I will now only use draft without further comment/tag. I find the draft mode better than a tag. The draft mode is an intention not to merge, but I can still see whether the technical requirements are met. The tag would always fail the corresponding action and it’s more cumbersome to dig out whether it failed due to the tag only or whether there are other technical blockers. |
I agree - I'm all for using the more precise/specific option and then falling back. You're also not the only person I've seen put "Do not merge" on things that weren't drafts. All this was intended as was a fallback when "waiting for upstream" or "needs discussion" doesn't quite fit and also they don't want it in drafts. But also yeah not attached to this PR so I'll eventually close it if nobody would use the tag. |
|
I think this is also useful for maintainers to be able to block PRs from contributors who might not know how to handle drafts, with the flexibility for other maintainers to release it. "Request changes" fulfills this to some extent, but requires that the original maintainer be the one to remove the block. |
|
I'm not sure either of those are actually issues. Maintainers can dismiss a blocked review from another maintainer. They can also change the draft status of a PR. |
Like I understand many folks don't see it this way, but for me dismissing someone elses block feels disrespectful so I'd feel uncomfortable doing so w/o their approval. Unless the block was a straight technical list of changes,
Also something I'd feel uncomfortable doing for someone else 'cause I don't want to assume I know what they intended. Especially since we've been wildly inconsistent on "are drafts reviewable?" I'd be more likely to leave a comment "hey, you may want to make this a draft" Which the point of this PR was a fallback label that could always be replaced with something more precise. |
|
Etiquette aside, the mechanics of this label are identical to the mechanics of draft status or a blocking/unblocking a review. The same etiquette would apply to adding or removing the label, I'd think |
I figure people would explain why something is labeled do not merge > and actually I just got my use case #27304 where it would have been very useful to prevent the merging while I was in the middle of working on what was a longish review - it definitely doesn't belong in drafts but I also don't know if the review is blocking w/o finishing it. |
PR summary
Since we already run a check that blocks merging on certain labels, added a specific "do not merge" for situations where it doesn't fit the other do not merge categories.
PR checklist