scientific-python / changelist

Prepare an automatic changelog from GitHub pull requests
BSD 3-Clause "New" or "Revised" License
8 stars 4 forks source link

Label required for merging instead of part of CI #49

Open dschult opened 1 year ago

dschult commented 1 year ago

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 commented 1 year ago

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?

bsipocz commented 1 year ago

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 commented 1 year ago

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

stefanv commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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]