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

Update labeler config for major version increment #2747

Closed

Conversation

moradology
Copy link
Contributor

@moradology moradology commented Jan 22, 2025

This PR simply prepares the repo to upgrade the major version of the labeler workflow (and fix all the workflow errors people are encountering). See docs here: https://github.com/actions/labeler?tab=readme-ov-file#updating-major-version-of-the-labeler

  1. Once verified (PRIOR TO MERGING) pull_request on line 4 of needs_release_notes.yml will need to be reverted to pull_request_target. This must happen to avoid permissions issues! Cf. https://github.com/actions/labeler?tab=readme-ov-file#recommended-permissions
  2. At this point things will fail again (the changes below won't exist so far as the github action is concerned). Ignore the failure. Merge.

This addresses the issue mentioned in #2744 and the unfortunate bug hanging up #2533 (among others)

@moradology
Copy link
Contributor Author

OK, here's verification:
image
image

@moradology
Copy link
Contributor Author

This was kind of confusing, so I'm going to avoid switching back to pull_request_target until I get some verification that my explanation/the validation of the fix here is acceptable

This was referenced Jan 22, 2025
@martindurant
Copy link
Member

I also tried something similar in my PR, following from discussion I found online. I have no idea what's really going on! It would be OK by me to merge something to see if it gets fixed, without 100% certainty, since it's definitely broken right now.

@moradology
Copy link
Contributor Author

OK, switched over to pull_request_target. This can be merged, at which point it should work in other PRs

@martindurant
Copy link
Member

@dstansby , perhaps?

- uses: actions/labeler@8558fd74291d67161a8a78ce36a881fa63b766a9 # v5.0.0
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to checkout the repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Failure to find .github/labeler.yml, which was the first error being encountered on #2533 and led me to looking into this

ref: ${{ github.event.pull_request.head.sha }}

- name: Run Pull Request Labeler
uses: actions/[email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we switch back to using the SHA instead? It will be updated find by dependabot, but it's generally recommended to use the SHA instead of a tag for security reasons.

@dstansby
Copy link
Contributor

I think a slightly simpler fix is at #2744 - if another maintainer wants to review and merge that instead I think it would work? Otherwise I left a couple of comments above on this PR, and I'd be happy to merge this PR once they're resolved.

@moradology
Copy link
Contributor Author

moradology commented Jan 22, 2025

🫡 Simpler is generally better, so I defer to the wisdom of others here! I do think the checkout is going to ultimately need to make its way in, however

@martindurant
Copy link
Member

Thanks for working on this @moradology - it looks like it's fixed now on main

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

Successfully merging this pull request may close these issues.

4 participants