fix(ci): replace sed version bump with npm version to prevent script corruption#125
fix(ci): replace sed version bump with npm version to prevent script corruption#125Kyzgor wants to merge 1 commit into
Conversation
fb9c66a to
a14220b
Compare
|
The |
zeevmoney
left a comment
There was a problem hiding this comment.
Correct fix — npm version is JSON-aware and only mutates the top-level field, and it strips a leading v from the tag. One small nit.
| run: | | ||
| sed -i "s/\"version\": \".*\"/\"version\": \"${{ github.event.release.tag_name }}\"/" package.json | ||
| cat package.json | ||
| run: npm version "${{ github.event.release.tag_name }}" --no-git-tag-version --allow-same-version --ignore-scripts |
There was a problem hiding this comment.
[LOW] Add a comment explaining the three flags
--no-git-tag-version, --allow-same-version, and --ignore-scripts are non-obvious to a future maintainer.
Suggestion: Prefix with e.g. # bump top-level version only; no git tag, allow re-runs, skip the version lifecycle script.
There was a problem hiding this comment.
Pull request overview
This PR fixes the Node SDK publish workflow’s version-bump step to avoid corrupting package.json by replacing a greedy sed rewrite with npm version, and restores the previously corrupted scripts.version entry.
Changes:
- Replace the CI
sed-based version bump withnpm version ... --no-git-tag-version --allow-same-version --ignore-scriptsto only update the top-levelversionfield. - Restore
package.json’sscripts.versionback tostandard-version.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
package.json |
Restores scripts.version to standard-version so release tooling isn’t broken. |
.github/workflows/node_sdk_publish.yaml |
Uses npm version (JSON-aware) instead of a greedy sed substitution during publish. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The publish workflow bumped the package version with a greedy sed that matched every "version": "..." entry in package.json, corrupting scripts.version (which the prepare-release lifecycle invokes via run-s). Use npm version, which is JSON-aware and only touches the top-level field, and restore the corrupted scripts.version back to "standard-version". A post-bump assertion fails the release if scripts.version is ever clobbered again. For the repo's bare-semver release tags the bump is equivalent; npm version additionally strips a stray leading "v" and validates semver (failing fast) where the old sed wrote the tag verbatim. Flags: --no-git-tag-version (no CI commit/tag), --allow-same-version (tolerate re-runs), --ignore-scripts (don't fire the version lifecycle in CI).
a14220b to
f6ec1b8
Compare
|
Thanks — pushed an update:
Rebased on |
What kind of change does this PR introduce?
Bug fix (CI / release tooling).
What is the current behavior? (link: CI workflow uses a regexp replace and changed the
versionscript #89)The publish workflow (
.github/workflows/node_sdk_publish.yaml) bumps the package version beforepublishing with a greedy
sed:This regex matches every
"version":key inpackage.json, not just the top-level field. Thescriptsblock contains"version": "standard-version"(astandard-versionlifecycle hookinvoked by
prepare-release:run-s reset-hard test cov:check doc:html version doc:publish). Thesedrun overwrites that script with the release version string. The corruption already happened(introduced in commit
2abe31f) and is still live onmaintoday —scripts.versionis"2.5.2"instead of"standard-version", so theversionstep ofprepare-releaseis broken.What is the new behavior (if this is a feature change)?
The bump step uses npm's JSON-aware tooling, which only mutates the top-level
versionfield:--no-git-tag-version— no commit/tag created in CI.--ignore-scripts— does not fire theversionlifecycle script (standard-version) during the bump.--allow-same-version— tolerates re-runs / a tag equal to the current version.package.json'sscripts.versionis also restored from the corrupted"2.5.2"back to"standard-version", and a one-line post-bump assertion in the same step fails the release ifscripts.versionis ever clobbered again. This matches the fix suggested in CI workflow uses a regexp replace and changed theversionscript #89.For the repo's bare-semver release tags the bump is equivalent to the old
sed; notenpm versionadditionally strips a stray leading
vand validates semver (failing fast) wheresedwrote thetag verbatim — a small improvement, not a behaviour change for current tags.
Other information:
Reproduced before/after (same input package.json, tag
9.9.9)What's NOT in this PR: no broader rewrite of the publish workflow, no
standard-versionautomated-release wiring, no dependency changes, no source changes. (A separate, unrelated issue:
prepare-releasealso referencesdoc:html/doc:publishscripts that aren't defined — out ofscope here.)
The
security/snyk (permit)check shows ERROR; this is an external/integration issue on fork PRs(this PR adds no dependencies and changes only CI config + a script string). A maintainer re-run or
waiver would clear it.
Fixes CI workflow uses a regexp replace and changed the
versionscript #89