-
Notifications
You must be signed in to change notification settings - Fork 5
Require core4 labels on pull requests before merging #105
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
Conversation
Co-authored-by: TJ Silver <15648334+tjsilver@users.noreply.github.com>
7a5ff92 to
d10b44a
Compare
d10b44a to
ad4fbb6
Compare
adamnfish-gu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be good to chat about how this will get rolled out to each (production?) repository, but I'm sure you've already considered this!
The trouble with the approach is that I imagine people are going to open a PR and then add the labels to that PR, which means this will always need manually re-running, because it triggers on the PR being opened.
We can add additional types to the PR trigger to improve the UX:
on:
pull_request:
types:
- labeled
- unlabeled
- ... others
I also think we can simplify the implementation a bit, as a I mention in another comment. Here's an example of the changes I talked about on a different branch, in case that's helpful?
https://github.com/guardian/.github/compare/require-core4-labels-alternative
Unfortunately because this is a centralised workflow, the usual triggers will not work, such as labeled, unlabeled, etc, as they take their context from the repo that the workflow is in, rather than the repo it is running in. That is why we had to come up with this solution here. |
Co-authored-by: adamnfish-gu <adam.fisher@guardian.co.uk>
| LABELS: ${{ toJSON(github.event.pull_request.labels.*.name)}} | ||
| with: | ||
| script: | | ||
| const labels = JSON.parse(process.env.LABELS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this on test-repocop-prs and the workflow failed on the creation of a PR without a label (as expected) and then re-ran automatically after I added a label.
tjsilver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are happy with the idea that this workflow will always fail on the first run when a user opens a PR (inevitably they will open it before before labelling), then it's good to go.
Workflow to require one of the core4 labels on pull requests before merging. It contains an extra don't know label at the moment, this was a request so that we can see which pull requests people have trouble labelling. We will remove this label in the future when it is no longer needed.