Repository navigation
provision-node input; README recipe and smoke job for running beside actions/setup-node - #11
Conversation
|
Run failed. View the logs →
|
There was a problem hiding this comment.
🟢 Approval recommended
Only a minor documentation grammar issue was found; the functional changes are small and are covered by the updated smoke workflow.
Pull request overview
This PR updates setup-nub’s optional package-manager shim behavior so that, when enabled, bare npm ci / npm install can be routed through Nub’s engine (while all other package-manager invocations keep using the pinned/real tools), and documents/tests that behavior.
Changes:
- Update
shim: trueto runnub pm shim --route-installsand emit a notice when the installed nub doesn’t support install routing. - Rewrite README guidance around an additive migration (keep
actions/setup-node, addsetup-nub, don’t change workflow commands). - Extend the smoke workflow to assert the routed vs non-routed behavior via a shim marker file.
File summaries
| File | Description |
|---|---|
| README.md | Updates documentation for routed npm ci/npm install behavior under shim: true. |
| action.yml | Passes --route-installs to nub pm shim and prints a notice when routing isn’t enabled. |
| .github/workflows/smoke.yml | Adds a smoke test that verifies the routed vs non-routed npm ci behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| With `shim: true`, the action runs `nub pm shim` and puts the shim directory first on PATH. In a project that pins a package manager (`packageManager` or `devEngines.packageManager`), `npm`, `pnpm`, and `yarn` in later steps run that pinned version, provisioned on demand, the same job corepack does. In an unpinned project they fall through to the runner's own tool. The shims do not route those commands into Nub's own installer; use `nub install` for that. | ||
| With `shim: true`, the action runs `nub pm shim --route-installs` and puts the shim directory first on PATH. Two things follow for later steps. | ||
|
|
||
| A bare `npm ci` or `npm install` in a project with a `package-lock.json` runs on Nub's engine, from that lockfile, with the flags npm's own CI invocations carry (`--ignore-scripts`, `--omit=dev`, `--no-audit`, `--no-fund`) translated or accepted. Nothing else in the workflow changes: `npm test`, `npm run`, `npx`, an install with a package name, and an install carrying a flag Nub does not translate all run the real npm as typed. So adding the action next to `actions/setup-node` is the whole migration: |
9443730 to
e62c87a
Compare
|
Run failed. View the logs →
|
colinhacks
changed the title
e62c87a to
f352d4d
Compare
|
Run failed. View the logs →
|
…tall A workflow that keeps setup-node adds setup-nub after it, with the same node-version, and replaces `npm ci` with `nub install --frozen-lockfile`. The smoke job proves it on all three runners: the install runs on Nub's engine from package-lock.json, bare node stays the version setup-node chose, and `npm run` still works.
f352d4d to
7bdc73b
Compare
|
Run failed. View the logs →
|
provision-node: false skips the Node provision step: nothing is fronted on PATH and the node-version output is empty, for a job where actions/setup-node already put the wanted Node on PATH. The smoke job for the beside-setup-node recipe writes a disagreeing .nvmrc first, so the check that bare node stays setup-node's would fail with the step on. A package-manager name as the cache value no longer prints a notice; it is a documented spelling.
|
Run failed. View the logs →
|
colinhacks
changed the title
The dep-glob output was written with a plain echo, so a second line broke GITHUB_OUTPUT with 'Invalid format'. Heredoc form, as pin-glob already used. A smoke job passes a two-line list.
|
Run failed. View the logs →
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No blocking issues — the functional change is sound and this PR's own CI already proves it end to end. What's left is documentation and coverage polish.
Reviewed changes — pullfrog has not reviewed this PR before (the only prior review is Copilot's, against 9443730, which the branch has since rewritten), so this pass covers the full diff rather than only the 7ca7b3c..78849ce delta.
- Added the
provision-nodeinput — defaults totrue;falsegates off the entireProvision Nodestep, so nothing is provisioned, no Node bin dir is fronted, and thenode-versionoutput comes back empty. - Documented a "Beside setup-node" recipe — keep
actions/setup-node, addsetup-nubwithprovision-node: false, swapnpm cifornub install --frozen-lockfile. - Fixed
dep-globfor a newline-delimitedcache-dependency-path— switched to the heredoc$GITHUB_OUTPUTform, matching thepin-globblock right below it. The previousecho "dep-glob=$dep_glob"wrote a malformed output file and failed the step outright for any multi-line value. - Dropped the
::notice::that fired whencachewas given a package-manager name; the value is still treated as truthy, only the log line is gone. - Added two smoke jobs —
multi-dep-path(a two-linecache-dependency-path) andadditive(three-OS:actions/setup-nodepins Node 20, a root.nvmrcof 22 serves as the negative control, andnub install --frozen-lockfileinstalls a hand-written npm lockfile).
Two things I checked rather than assumed. steps.provision is referenced in exactly one place repo-wide — the node-version output at action.yml:83 — so skipping the step cannot starve any other step, and the documented "output empty" behavior falls out for free. And because smoke.yml triggers on pull_request, the new jobs gate this PR: additive is already green on ubuntu and windows, which settles the node_modules/.nub-engine marker, the --frozen-lockfile flag, the hand-written file: lockfile fixture, and the exact-equality [[ "$out" == "42" ]] capture under Git Bash. None of those needed flagging in the end.
ℹ️ The "Node on the global PATH" section still describes PATH fronting as unconditional
README.md:59 opens with "setup-nub provisions a Node toolchain and adds its bin dir to the global PATH", and the "Which version is fronted" list enumerates three cases — explicit input, project pin, no pin. provision-node is the one input whose entire purpose is to suppress that mechanism, and it appears nowhere in the section a reader consults to understand PATH behavior.
Technical details
# `provision-node` is absent from the section that documents PATH fronting
## Affected sites
- `README.md:59` — asserts unconditionally that the action "provisions a Node toolchain and adds its bin dir to the global `PATH`".
- `README.md:61-66` — the "Which version is fronted" list covers explicit `node-version` / project pin / no pin, but not `provision-node: false`.
- `README.md:67` — "The remaining nuance vs setup-node" now understates the set of nuances.
## Required outcome
- A reader who lands on "Node on the global PATH" learns that the fronting can be turned off entirely, without having to find the new "Beside setup-node" section or the inputs table.
## Suggested approach (optional)
- Add a fourth bullet to the "Which version is fronted" list along the lines of: **With `provision-node: false`** — nothing is provisioned and nothing is fronted; whatever is already on `PATH` (typically `actions/setup-node`'s Node) stays, and the `node-version` output is empty.
- Soften the opening sentence's "provisions ... and adds" to note the default, e.g. "by default".ℹ️ Nitpicks
action.yml: settingnode-version(ornode-version-file) together withprovision-node: falsesilently ignores the version input — the step that consumes it never runs. The action already warns on subtler divergences (the deprecatedversionalias, the pin-vs-PATH mismatch ataction.yml:604), so a one-line::warning::when both are set would fit the existing style and catch a plausible mis-migration.
Claude Opus ｜ 𝕏
…asserts the empty output
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the 78849ce..7bbcef6 delta, which is entirely the two fix-ups for the inline threads left on the previous pass.
- Asserted the empty
node-versionoutput in theadditivesmoke job — theuses: ./step now carriesid: nub, and the script below it fails the job ifsteps.nub.outputs.node-versionis non-empty. - Corrected the
Provision Nodestep's intent comment — "This runs UNCONDITIONALLY" is replaced by a description that namesprovision-node: falseas the opt-out and restates what it suppresses (no provision, nothing fronted, empty output).
I checked that the new assertion can actually fail rather than passing vacuously: smoke.yml:159 and smoke.yml:185 assert that same output equals 20* in the pin-resolution and eager-provision jobs, so the output is demonstrably populated whenever the provision step runs. If provision-node: false ever stopped gating the step, line 433 would catch it. The additive job is already green on windows-latest at 7bbcef6, which settles empirically that a skipped composite step yields an empty action-level output.
Both prior threads are replied to and resolved. The one item from the last pass that the new commits don't touch is the Node on the global PATH section in README.md, which still describes PATH fronting as unconditional — it stays on the record from the previous review and is not a merge blocker.
Claude Opus ｜ 𝕏
Adds
provision-node(defaulttrue). Withfalsethe Node provision step is skipped: nothing is fronted on PATH and thenode-versionoutput is empty, for a job whereactions/setup-nodealready put the wanted Node on PATH.nubstill resolves the project's pin at its own invocation.README gains a "Beside setup-node" recipe: keep
actions/setup-node, addsetup-nubwithprovision-node: false, replacenpm ciwithnub install --frozen-lockfile. A smoke job proves it on ubuntu, macOS and Windows against a.nvmrcthat disagrees with setup-node's version.A package-manager name as the
cachevalue no longer prints a notice.nubjs/setup-node builds on this input.