Skip to content

Fix the sass_api release #2557

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

Merged
merged 1 commit into from
Apr 2, 2025
Merged

Fix the sass_api release #2557

merged 1 commit into from
Apr 2, 2025

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Apr 1, 2025

Pub.dev just launched an update that requires GitHub publish actions
to be run from a tag that matches the version number of the published
package.

@nex3 nex3 requested a review from Goodwine April 1, 2025 22:34
@nex3 nex3 force-pushed the sass-api-release branch from 13b7344 to 9e32f66 Compare April 1, 2025 22:36
# This should be /-separated rather than hyphenated, but pub.dev doesn't
# currently allow that (dart-lang/pub-dev#8690).
- run: git tag sass-api-${{ steps.sass-api-version.outputs.version }}
- run: git push --tag
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to push with the implicit ${{ github.token }} that comes from github actions, you need to add permissions to the job definition:

permissions:
  content: write

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, another problem I remember is that pushing with implicit token won't trigger another workflow. So you probably should use ${{ secrets.GH_TOKEN }} in checkout action, that it will trigger the workflow for new tag push.

Copy link
Contributor Author

@nex3 nex3 Apr 1, 2025

Choose a reason for hiding this comment

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

If you want to push with the implicit ${{ github.token }} that comes from github actions, you need to add permissions to the job definition:

permissions:
  content: write

Isn't that inherited from the invocation in ci.yml? deploy_sass_parser is pushing a tag without explicit local permissions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, another problem I remember is that pushing with implicit token won't trigger another workflow. So you probably should use ${{ secrets.GH_TOKEN }} in checkout action, that it will trigger the workflow for new tag push.

Done.

Copy link
Contributor

@ntkme ntkme Apr 1, 2025

Choose a reason for hiding this comment

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

Isn't that inherited

Permission inheritance in reusable workflow is a bit complicated. The permission declared in parent workflow defines the maximum permission child workflows can ask for, but if a child workflow did not explicitly ask for the permission, it won't get inherited from parent automatically.

https://docs.github.com/en/actions/sharing-automations/reusing-workflows#access-and-permissions

Anyways, better to use a PAT so that it guarantee the push event triggers a new workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's now using the same setup as deploy_sass_parser including the token it uses for checkout so it should work.

@nex3 nex3 force-pushed the sass-api-release branch from 9e32f66 to d8a831f Compare April 1, 2025 23:11
@Goodwine Goodwine requested a review from ntkme April 1, 2025 23:23
@nex3 nex3 force-pushed the sass-api-release branch from d8a831f to 8246553 Compare April 1, 2025 23:23

jobs:
deploy_sass_api:
if: "github.event.repository.fork == false"
Copy link
Member

Choose a reason for hiding this comment

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

Are these quotes necessary? IIRC quotes are only needed when the text contains a *

same below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a number of active YAML characters. You're right that this doesn't need them, but I prefer to use them for anything with ${{ }} interpolation just to avoid needing readers to remember when { is or isn't an active YAML character.

echo "version=$(cat pkg/sass_api/pubspec.yaml | sed -nE 's/version: (.*)/\1/p')" | tee --append "$GITHUB_OUTPUT"
# This should be /-separated rather than hyphenated, but pub.dev doesn't
# currently allow that (dart-lang/pub-dev#8690).
- run: git tag sass-api-${{ steps.sass-api-version.outputs.version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely sure, but another way this might be broken is that git might complain that "user" is not setup. Maybe we need to add these before creating the tag:

git config user.email 
git config user.name 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe lightweight tags don't include any author information. (Note that the git user isn't set when tagging sass-parser below.)

@nex3 nex3 force-pushed the sass-api-release branch from 8246553 to a06eecc Compare April 1, 2025 23:39
Pub.dev just launched an update that requires GitHub publish actions
to be run from a tag that matches the version number of the published
package.
@nex3 nex3 force-pushed the sass-api-release branch from a06eecc to 3577082 Compare April 2, 2025 00:15
@nex3 nex3 requested a review from Goodwine April 2, 2025 00:32
@nex3 nex3 merged commit f6fea3a into main Apr 2, 2025
39 checks passed
@nex3 nex3 deleted the sass-api-release branch April 2, 2025 22:51
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.

3 participants