-
Notifications
You must be signed in to change notification settings - Fork 43
Ensure the release asset can be attached properly #340
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
| permissions: | ||
| contents: read | ||
| contents: write | ||
| packages: 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.
Does this also need to be updated to write as done in #345 (and currently underway updating across other plugin repos)?
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.
So I know we have this set to write in some of our other repos but I don't think it needs to be, though I guess I don't know that for sure and hard to test until we do an actual release. Happy to update that just to be safe if we want
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.
Maybe better to start with just this change and then can add the packages: write update later if the run fails again?
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 going to go ahead and update all the other PRs I opened to remove the packages: write change and merge those in so we get everything to the same contents:write/packages:read state and can iterate everything together in the future if needed.
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.
Though I guess also worth clarifying here before I march off and make broad changes, is the id-token: write bit here something we should replicate across the other deploy actions?
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.
Looked at a few other plugins that have had successful releases (including attaching a release asset) since adding custom permissions. The three I looked at (Safe SVG, Convert to Blocks, and Autoshare for Twitter all have contents: write and packages: write but none of them have id-token: write, so I don't think we need that
Description of the Change
After the last release (2.8.4), an error occured when our GitHub Action tried to attach the final release asset to the release (see https://github.com/10up/simple-local-avatars/actions/runs/16271766801). This is due to permission changes we made in #337. This PR updates the permissions there to ensure this works properly.
How to test the Change
Nothing really to test here
Changelog Entry
Credits
Props @dkotter
Checklist: