Plugin/Status check for "do not merge" label?

[platinumazure]

I think it would be awesome to create a plugin which creates a pending status check whenever "do not merge" is added to a PR, and removes the status check when the label is removed.

I suggest "pending" instead of "failed" since in most cases, users might not be at fault. We can always add comments to the PR to indicate if a user needs to change something. But for the most part, implementation details don't matter to me.

[not-an-aardvark]

I think this would be a good idea. One potential downside is that a user without write access can't add the do not merge label to their own PRs. As an alternative, maybe we could check if the issue title starts with "WIP".

[platinumazure]

Hmm, actually, maybe this should be created as a standalone Probot plugin. If it were configurable to accept one or more labels and an issue title regex, it could be used by non-ESLint projects too.

This leads to the next obvious question: Has someone done this already? I'll have to investigate that later.

[gyandeeps]

Their is already one available: https://github.com/gr2m/wip-bot.

Maybe we can make the release monitor do this as that PR is not in a mergable state? (Don't feel too strongly about it)

[not-an-aardvark]

To me, it seems better to use the existing integration rather than adding more functionality to release-monitor.

[platinumazure]

I explored enhancing the existing plugin, but the maintainer wasn't interested. So either we should just create the plugin in this repo, or one of us could create the plugin standalone and consume here while also allowing others to use it. I'm not strongly attached to either option at this point and we could probably pivot later if needed.

[gyandeeps]

I guess we can enhance the commit-message plugin to put a pending state if the commit message is WIP. That way we will not create a separate status for WIP.

Commit-message plugin:

Success: If message is correct
Failure: If the message is not correct
Pending: If message is correct and its WIP

[platinumazure]

I don't think it makes sense to enhance commit-message because we could use the PR title for WIP and the commit message for commit-message (or vice versa), plus we should also support "do not merge" label. So I think it makes sense as a separate plugin.

…

On Dec 25, 2017 9:05 PM, "Gyandeep Singh" ***@***.***> wrote: I guess we can enhance the commit-message plugin to put a pending state if the commit message is WIP. That way we will not create a separate status for WIP. Commit-message plugin: Success: If message is correct Failure: If the message is not correct Pending: If message is correct and its WIP — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#51 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWeipCEonHlCdrg_tM_9Fwo0xq_UN_ks5tEGKRgaJpZM4RLzFf> .

[not-an-aardvark]

I agree, I think it would be better to use a separate plugin. I don't think it's worth making an existing plugin more complex when the functionality isn't really related.

If we want to avoid too many status checks, we could avoid creating the WIP status unless the PR has been marked as WIP previously.

[gyandeeps]

Yeah, after thinking about it some more I dont think its a good idea to enhance commit-message.

[platinumazure]

This isn't accepted yet (what's our process for accepting things here, anyway?), but I've started working on this. I'm also working on extracting PR/commit message detection logic into a common utility, since there are already 2 plugins with the same logic and this plugin would be the third to do so.

Although, that leads to a question: Should we also flag PRs in which any commit has WIP in it? On the one hand, it could help flag some less common WIP cases; on the other hand, it could also ask contributors to edit their commit history needlessly, and we could also just use the "do not merge" label when we see those cases. Any thoughts?

[not-an-aardvark]

what's our process for accepting things here, anyway?

I don't think we have a formal process. So far, it seems like we've effectively been accepting anything that someone is willing to implement as long as no one objects to the change.

Should we also flag PRs in which any commit has WIP in it?

Based on our current recommendations, PR authors should push new commits to a PR rather than amending commits. So if a PR is marked as WIP at some point, the author will make the necessary changes by adding additional commits, leaving the old "WIP" message in the commit history. I think it would be better for the status check to allow this rather than requiring the author to amend the history.

It seems like there are two potential goals that we could have for this plugin (feel free to correct me if I've missed any):

1. The PR author wants to indicate to the team that the PR is in progress, to prevent the PR from being merged prematurely.
2. The team wants to avoid accidentally putting commits with "WIP" in the commit history.

If we're only trying to accomplish goal 1, I'm wondering if it would be better to exclusively check the title of the PR (ignoring commit messages), regardless of the commit count. For example, if I make a PR with one commit, and then I spot an error in the PR, it would be convenient to be able to mark it as WIP from the GitHub UI, without needing to amend my commit from the command line.

If we're also trying to accomplish goal 2, then using the existing commit message logic makes sense.

[platinumazure]

If we're also trying to accomplish goal 2, then using the existing commit message logic makes sense.

We use squash-and-merge in the UI, right? This would only be a concern if someone merges the commit to master locally and pushes. (Do we even allow pushes to master, or just force pushes?)

[not-an-aardvark]

I was referring to the existing logic used by the commit-message check, which predicts what the commit message will be after a PR is squash-merged.

We currently allow pushes by repository admins, which is needed for the release script to push commits without making a PR. We don't allow force-pushes by anyone (although repository admins have the ability to change the branch protection setting to override this if necessary).

[platinumazure]

I've been working on this on and off. Right now I have a local branch with most of the logic laid out, but I haven't written any unit tests yet (and will probably discover some bugs once I begin that exercise). Hoping to get something up this week.