-
Notifications
You must be signed in to change notification settings - Fork 2
Migrate from NPM tokens to trusted publisher workflow. Fix tests. #313
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
|
0e64ae5 to
42c7efa
Compare
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.
Approving ✅. Should we test-run this on the current branch before merging?
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "20" |
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.
.nvmrc has version v18.13.0
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.
👍
| needs: [check-version-change] | ||
| if: needs.check-version-change.outputs.did-version-change == 'true' | ||
| steps: | ||
| - uses: actions/checkout@v4 |
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.
Also you might want to check actions/checkout@v4 here and v2 on line 46.
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.
Good catch! Thanks.
NPM is sunsetting the old tokens that we were using to publish to NPM. They advise using trusted publishers for publishing via CI/CD instead.
This change will also run the unit tests before publishing. Apparently we haven't ran them in a while, so they needed fixing. As part of the fix, I noticed VuiSummary produced invalid DOM (nesting divs inside of a p tag), so I fixed that too.