fix: npm publish workflow - #113
Conversation
Stop ignoring package-lock.json and commit it so `npm ci` (used in the new GitHub Actions workflows) has a lockfile to install from. It was previously untracked/ignored (since #86), which made sense before npm ci existed, but now blocks reproducible CI installs. This has no effect on the published package, since "files": ["lib"] already excludes the lockfile from the npm tarball. Co-authored-by: Cursor <cursoragent@cursor.com>
The lockfile committed in adfee80 was generated inside a sandbox whose npm config defaults to an internal Netflix Artifactory registry mirror, so every "resolved" tarball URL pointed at artifacts.netflix.net instead of registry.npmjs.org. That's unreachable from GitHub-hosted runners and external contributors, which would break `npm ci` for anyone outside Netflix's network. Versions and integrity hashes are unchanged; only the resolved host differs. Co-authored-by: Cursor <cursoragent@cursor.com>
Node 24 fails due to a known restify/spdy incompatibility (removed
process.binding('http_parser') internal API) unrelated to this
package; the job's name implies its failures shouldn't block merges,
so make that actually true.
Co-authored-by: Cursor <cursoragent@cursor.com>
0777131 to
04b0cb6
Compare
Job-level continue-on-error only stops the failure from cascading to the overall workflow run/needs-dependent jobs; it doesn't change the job's own reported check conclusion, so test-optional still showed as a failing check on PRs. Moving it to the individual steps makes the job itself report success while still surfacing Node 24 failures in the logs. Co-authored-by: Cursor <cursoragent@cursor.com>
wesleytodd
left a comment
wesleytodd
left a comment
There was a problem hiding this comment.
A few changes that I think might be necessary, but mostly questions. I will leave this as request changes, but since I already was behind on reviewing it if you are confident in the answers to my questions don't let that block merging. This is low risk and we can change anything forward as necessary.
|
|
||
| concurrency: | ||
| group: npm-publish-${{ github.event.release.tag_name || github.run_id }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
Might be worth a comment on why we want these settings here. While I agree with it for the tag_name case, the fallback to run_id seems odd so I want to make sure I understand the goal.
There was a problem hiding this comment.
group requires a value and I picked run_id as fallback when tag_name is not present - this can happen on manual triggers, dry run/testing purposes.
| - uses: actions/checkout@v7 | ||
| with: | ||
| ref: ${{ github.event.release.tag_name || github.ref }} | ||
| persist-credentials: false |
There was a problem hiding this comment.
What does this do in this context? I don't think I have ever seen this be necessary, so just want to make sure I understand why it is here.
There was a problem hiding this comment.
This removes the github access token after the repo is cloned
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/restify/errors.git" | ||
| "url": "git+https://github.com/restify/errors.git" |
There was a problem hiding this comment.
Not sure why we need this change, seems equivalent and so unnecessary.
There was a problem hiding this comment.
npm docs recommendation: https://docs.npmjs.com/cli/v12/configuring-npm/package-json#repository
Co-authored-by: Cursor <cursoragent@cursor.com>
79077e5 to
31122dd
Compare
2 participants
npm-publish.yml workflow for enabling npm trusted publishing.
Notes: