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

chore: remove pr labeler requirement #10081

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

alextran1502
Copy link
Contributor

Remove pr label enforcing job since it is now done through CI

bo0tzz
bo0tzz previously requested changes Jun 10, 2024
Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still keep this job as an enforcement in case the other one fails, as well as if the changes don't result in that one automatically applying a label.

@alextran1502
Copy link
Contributor Author

You will get no green checkmark automatically though

@zackpollard
Copy link
Contributor

I think we should still keep this job as an enforcement in case the other one fails, as well as if the changes don't result in that one automatically applying a label.

The two don't work together, the label gets applied too late and then the label check job doesn't run because it won't be triggered by the bot adding a label.

@bo0tzz
Copy link
Member

bo0tzz commented Jun 10, 2024

Ah, because the workflow_run won't be triggered due to that annoying loop prevention, ugh... Any ideas for how we can work around that? If we can I think it would still be worth having this enforcement. As an alternative, maybe the enforcement can be baked into the other job?

@zackpollard
Copy link
Contributor

Ah, because the workflow_run won't be triggered due to that annoying loop prevention, ugh... Any ideas for how we can work around that? If we can I think it would still be worth having this enforcement. As an alternative, maybe the enforcement can be baked into the other job?

We talked about it earlier and really it's not a big deal if it doesn't get applied, we can always sort any missing a label when we come to making the release notes

@bo0tzz
Copy link
Member

bo0tzz commented Jun 10, 2024

we can always sort any missing a label when we come to making the release notes

Definitely true, but IMO it's much easier to do that when the context of the PR is still fresh.

@jrasm91 jrasm91 dismissed bo0tzz’s stale review June 10, 2024 16:59

Going to test without it and see how it goes

@jrasm91 jrasm91 merged commit 4698c39 into main Jun 10, 2024
22 checks passed
@jrasm91 jrasm91 deleted the chore/remove-pr-label-enforcing branch June 10, 2024 16:59
Comment on lines -6 to -9
workflow_run:
workflows: ["Pull Request Labeler"]
types:
- completed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This very condition was supposed to re-run the "Enforce PR labels" after "Pull Request Labeler". At least, that's how it worked in my fork.

So what's different? In the PR discussion a loop prevention is mentioned. Does that prevent the desired behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure really, I think we basically decided it wasn't worth investigating when the labels aren't actually "mandatory" they're just a really nice help and the other action should catch most cases anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. When I implemented it this way, I felt a little dirty about the dependency across workflows. Less complexity is a good thing, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants