-
-
Notifications
You must be signed in to change notification settings - Fork 220
ci: ensure the auto-created release-please action runs CI
#713
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
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.
Pull request overview
This PR updates the release-please workflow to ensure that automatically created release PRs trigger CI checks. By switching from the default GITHUB_TOKEN to WORKFLOW_PUSH_BOT_TOKEN, PRs created by the release-please action will now properly trigger CI workflows, preventing releases from being created without proper validation.
Key Changes:
- Replaced
GITHUB_TOKENwithWORKFLOW_PUSH_BOT_TOKENfor the release-please action - Removed explicit
contents: writeandpull-requests: writepermissions (redundant with bot token) - Maintained
id-token: writepermission for npm provenance support
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: release-please | ||
|
|
||
| permissions: | ||
| contents: write |
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'm not sure about this, but per https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions, "If you specify the access for any of these permissions, all of those that are not specified are set to none", which means contents will be none for steps that use GITHUB_TOKEN? Docs for trusted publishing (https://docs.npmjs.com/trusted-publishers#step-2-configure-your-cicd-workflow) have contents: read in the example, so perhaps it would be safer to change write -> read instead of just removing the line?
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 think it would still work well if we remove contents: write or contents: read for the following reasons:
You can use
permissionsto modify the default permissions granted to theGITHUB_TOKEN
According to https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions, the permissions field applies to GITHUB_TOKEN, not to Personal Access Tokens (fine-grained or classic) such as WORKFLOW_PUSH_BOT_TOKEN. (This change now removes the use of GITHUB_TOKEN.)
As far as I know, the permissions for WORKFLOW_PUSH_BOT_TOKEN are set when the token is created, not by the explicit permissions field in GitHub Actions. For example, with the following token settings (the example below shows a classic token):
Docs for trusted publishing (https://docs.npmjs.com/trusted-publishers#step-2-configure-your-cicd-workflow) have contents: read in the example, so perhaps it would be safer to change write -> read instead of just removing the line?
As far as I know, content: write or contents: read aren't related to OIDC; only id-token is:
For example, when I publish my personal open-source packages, I only use id-token: write, and it works fine without contents: read.
Also, in https://github.com/eslint/css/pull/330/changes, we removed the contents: write field as shown below, yet the auto-created workflow was generated correctly and the CI ran successfully.
Given the reasons above, I think this would work well; however, if you’re still unsure, I’m happy to follow your guidance and update the workflow to use contents: read.
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.
According to https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions, the
permissionsfield applies toGITHUB_TOKEN, not to Personal Access Tokens (fine-grained or classic) such asWORKFLOW_PUSH_BOT_TOKEN. (This change now removes the use ofGITHUB_TOKEN.)
Yes, but only one step uses WORKFLOW_PUSH_BOT_TOKEN, while the others still use GITHUB_TOKEN?
Also, in https://github.com/eslint/css/pull/330/changes, we removed the
contents: writefield as shown below, yet the auto-created workflow was generated correctly and the CI ran successfully.
In that change, we entirely removed permissions, in which case I believe contents defaults to read. Also, the steps are different than here and maybe those steps don't require contents: read.
For example, when I publish my personal open-source packages, I only use
id-token: write, and it works fine withoutcontents: read.
Then maybe it isn't true that trusted publishing requires contents: read or that specifying any permissions automatically sets the others to none. However, I think it's safer to go by the docs and specify contents: read.
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.
However, I think it's safer to go by the docs and specify contents: read.
First of all, I followed your suggestion and updated it in 9dc5ae9.
Just a small note: contents: read would matters for private repositories (since the read permission is also restricted in private repositories) and can affect some resuable GitHub Actions workflows or OIDC. However, since the repositories where we publish packages are public, the behavior would be the same whether it’s specified as contents: read or not. That said, being explicit could also be a good option :)
nzakas
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.
LGTM. Thanks.
Prerequisites checklist
What is the purpose of this pull request?
This PR follows up on eslint/css#330.
In this PR, I've ensured the auto-created
release-pleaseaction triggers CI.Problem
Currently, the auto-created PR from the
release-pleaseaction does not trigger CI, as shown below:For example: eslint/rewrite#336
This can result in CI not running and may lead to issues like eslint/rewrite#308 if the check is missing.
Solution
I've used
secrets.WORKFLOW_PUSH_BOT_TOKENinstead of the defaultsecrets.GITHUB_TOKEN, following the same approach described in eslint/css#330.Also, the
permissionsforcontentsandpull-requestsis no longer necessary becausesecrets.WORKFLOW_PUSH_BOT_TOKENalready grants the required permissions, so I removed it.FYI: the
tokeninput reference: https://github.com/googleapis/release-please-action?tab=readme-ov-file#action-inputsTest
I've tested it in my forked repository (using
rewriterepository), and it works as expected:lumirlumir/fork-rewrite#9
What changes did you make? (Give an overview)
This PR follows up on eslint/css#330.
In this PR, I've ensured the auto-created
release-pleaseaction triggers CI.Related Issues
Ref: eslint/css#330
Is there anything you'd like reviewers to focus on?
N/A