Skip to content

chore: update NPM publishing #465

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.

Sign up for GitHub

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

Jump to bottom
Merged
bestbeforetoday merged 1 commit into from
Oct 1, 2025
Merged

chore: update NPM publishing #465

bestbeforetoday merged 1 commit into from
Oct 1, 2025

Conversation

@ryjones
Copy link
Contributor

@ryjones ryjones commented Sep 27, 2025 •
edited
Loading

fixes #464

@ryjones ryjones requested a review from a team as a code owner September 27, 2025 02:41
jwagantall
jwagantall previously approved these changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@jwagantall jwagantall left a comment

Choose a reason for hiding this comment

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

LGTM overall but would be nice to have a maintainer double checking..

bestbeforetoday
bestbeforetoday requested changes Sep 30, 2025
View reviewed changes
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

The only changes in this commit actually required for npm publishing are:

  1. The explicit permissions for the publishnpm job.
  2. Removal of the NODE_AUTH_TOKEN environment variable in the npm publishing step, since trusted publishing is used instead of an auth token.

An additional step does need to be added between the actions/setup-node and npm publish steps:

      # Ensure npm 11.5.1 or later for trusted publishing support
      - name: Update npm
        run: npm install -g npm@latest

The npm version bundled with Node 18 does not support trusted publishing so the npm version needs to be updated explicitly. I believe that npm v11 requires Node version ^20.17.0 || >=22.9.0 so the Node version specified with actions/setup-node will need to be updated. I would suggest using node-version: "lts/*" to pick the latest LTS release.

I don't think we should change any of the other jobs in this workflow since they deal with Docker publishing rather than npm publishing.

.github/workflows/release.yaml Outdated
Comment on lines 12 to 19
# ---- Global permissions for Trusted Publishing & attestations ----
# id-token:write is required for OIDC (npm trusted publishing, keyless attestations)
# packages:write for GHCR; attestations:write for GitHub artifact attestations (optional but recommended)
permissions:
contents: read
packages: write
id-token: write
attestations: write
Copy link
Member

@bestbeforetoday bestbeforetoday Sep 30, 2025

Choose a reason for hiding this comment

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

Each of the jobs within the workflow defines required permissions so there is no need to define permissions for the workflow. It would be OK to define just read permission as a minimal default:

permissions:
  contents: read

Copy link
Contributor Author

@ryjones ryjones Oct 1, 2025

Choose a reason for hiding this comment

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

done

.github/workflows/release.yaml Outdated
find . -type f -name 'fabric-*.tgz' -exec npm publish {} \;
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
find . -type f -name 'fabric-*.tgz' -print0 | xargs -0 -I{} npm publish {} --provenance --access public
Copy link
Member

@bestbeforetoday bestbeforetoday Sep 30, 2025

Choose a reason for hiding this comment

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

I don't think there is any need for this change. The --exec option to find already invokes the command for each match, without needing the additional xargs command. The --provenance option to npm publish is implicit when trusted publishing is used. The --access option is unnecessary since publishing retains the (default) public visibility of the currently published package.

Copy link
Contributor Author

@ryjones ryjones Oct 1, 2025

Choose a reason for hiding this comment

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

reverted

@ryjones ryjones force-pushed the npm-publish branch 3 times, most recently from 4e3f801 to cfc993b Compare October 1, 2025 13:25
@ryjones
Copy link
Contributor Author

ryjones commented Oct 1, 2025

@bestbeforetoday thanks - I think I have all of your feedback rolled into it

@ryjones ryjones force-pushed the npm-publish branch 2 times, most recently from 5576a86 to 41edabe Compare October 1, 2025 14:06
bestbeforetoday
bestbeforetoday requested changes Oct 1, 2025
View reviewed changes
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

One functional issue and some minor tidy-up suggestions.

.github/workflows/release.yaml Outdated
Comment on lines 12 to 14
# ---- Global permissions for Trusted Publishing & attestations ----
# id-token:write is required for OIDC (npm trusted publishing, keyless attestations)
# packages:write for GHCR; attestations:write for GitHub artifact attestations (optional but recommended)
Copy link
Member

@bestbeforetoday bestbeforetoday Oct 1, 2025

Choose a reason for hiding this comment

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

I suggest removing these comments since they do not match the permissions statements below.

.github/workflows/release.yaml Outdated
steps:
- uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 # v5.0.0
with:
node-version: "18.x"
Copy link
Member

@bestbeforetoday bestbeforetoday Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The latest versions of npm (in the following step) will not work with Node 18. I suggest the latest LTS release (currently 22 and soon to be 24).

Suggested change
node-version: "18.x"
node-version: "lts/*"

.github/workflows/release.yaml Outdated
- run: |
- name: Publish packages with provenance (OIDC)
# No NODE_AUTH_TOKEN needed when Trusted Publishing is enabled.
# --provenance tells npm to attach SLSA provenance to the package. [oai_citation:1‡The GitHub Blog](https://github.blog/security/supply-chain-security/introducing-npm-package-provenance/?utm_source=chatgpt.com)
Copy link
Member

@bestbeforetoday bestbeforetoday Oct 1, 2025

Choose a reason for hiding this comment

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

I suggest removing this comment line since --provenance is not specified in the command-line below.

@ryjones
chore: update NPM publishing
4f6e417 
fixes #464

Signed-off-by: Ry Jones <[email protected]>
@ryjones
Copy link
Contributor Author

ryjones commented Oct 1, 2025

@bestbeforetoday done

bestbeforetoday
bestbeforetoday approved these changes Oct 1, 2025
View reviewed changes
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

@bestbeforetoday bestbeforetoday merged commit e85a01d into main Oct 1, 2025
8 checks passed
@bestbeforetoday bestbeforetoday deleted the npm-publish branch October 1, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bestbeforetoday bestbeforetoday bestbeforetoday approved these changes

@jwagantall jwagantall jwagantall left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate NPM workflows to Trusted Publishing

4 participants

@ryjones @bestbeforetoday @jwagantall