Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Label required for merging instead of part of CI #49

Open
dschult opened this issue Oct 11, 2023 · 6 comments
Open

Label required for merging instead of part of CI #49

dschult opened this issue Oct 11, 2023 · 6 comments
Labels
type: Enhancement New feature or request

Comments

@dschult
Copy link

dschult commented Oct 11, 2023

We've had a couple of confused first PR submitters who think the red-x of not having a label means that they did something wrong. It might be better to make having a label be a requirement for merging (since it is assigned by the maintainer who presumably will merge it) rather than a Github-Action checking whether there is a label.

How hard would it be to configure the label check as a merge requirement rather than a CI action?

@lagru
Copy link
Member

lagru commented Oct 13, 2023

Thanks for the suggestion. I get why this can be a confusing contributor experience. I'm not sure how easy it is to throw a wrench in the merge process but that would be ideal.

Maybe we could use a submitted review (pull_request_review, submitted) as a trigger for the job? For most projects that's bound to happen before merge and would likely come from someone that wouldn't be confused by the failure?

@lagru lagru added the type: Enhancement New feature or request label Oct 13, 2023
@bsipocz
Copy link
Member

bsipocz commented Oct 13, 2023

I really don't like PR/issue templates, but having different, label setting templates for the users to choose may remove that confusion.
Then the maintainers can correct if a PR was miscategorised.

@bsipocz
Copy link
Member

bsipocz commented Oct 13, 2023

But triggering on review (both submitted, but also edited and dismissed) sounds like an excellent solution.

@stefanv
Copy link
Member

stefanv commented Oct 31, 2023

I just looked into this a bit, and currently GH has very specific behaviors to choose from when it comes to merge blocking. As such, the only places to inject this (not as a standard check) would be in a merge queue, or on the main branch, after merge.

@lagru's suggestion may work: we only run the label check once the PR has been approved: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-when-a-pull-request-is-approved

I don't know if this will sufficiently block merging, but it's worth a try.

@stefanv
Copy link
Member

stefanv commented Oct 31, 2023

This does seem to do the trick: it only runs the label checker after the first review is made:

https://github.com/stefanv/test-labeler/blob/main/.github/workflows/label-check.yaml

@jarrodmillman
Copy link
Member

jarrodmillman commented Nov 13, 2023

This does seem to do the trick: it only runs the label checker after the first review is made:

It also makes every PR fail unless you add the label before the review.
2023-11-13T09:45:35,169937910-08:00

It fails on

  pull_request_review:
    types: [submitted]

and then passes on

  pull_request:
    types: [reopened, labeled, unlabeled]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants