chore(repo): remove populate-storage scripts, consolidate into nx-release#34814
chore(repo): remove populate-storage scripts, consolidate into nx-release#34814FrozenPandaz wants to merge 6 commits intomasterfrom
Conversation
FrozenPandaz
requested review from
a team,
AgentEnder,
JamesHenry,
jaysoo,
meeroslav and
vsavkin
as code owners
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
View your CI Pipeline Execution ↗ for commit cd7c0e5
☁️ Nx Cloud last updated this comment at |
FrozenPandaz
force-pushed
the
chore/remove-populate-storage-scripts
branch
from
0b9b010 to
07572df
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
FrozenPandaz
force-pushed
the
chore/remove-populate-storage-scripts
branch
2 times, most recently
from
737d46d to
6f52d1e
Compare
…ease Remove the now-redundant populate-storage.js and run-populate-storage.mjs scripts. The release logic is inlined directly in global-setup.ts, and the populate-local-registry-storage task is simplified to an orchestration point that delegates to nx-release via dependsOn. When running e2e tests outside of Nx (e.g. Jest directly), global-setup now auto-starts verdaccio if it's not already running.
FrozenPandaz
force-pushed
the
chore/remove-populate-storage-scripts
branch
from
6f52d1e to
b9ad989
Compare
FrozenPandaz
force-pushed
the
chore/remove-populate-storage-scripts
branch
from
dae4531 to
74622cf
Compare
Add nx-release-npm target for real npm publishes (--local false). Keep nx-release for local verdaccio releases via populate-local-registry-storage. Update publish.yml to use the new target.
FrozenPandaz
force-pushed
the
chore/remove-populate-storage-scripts
branch
from
176ac50 to
592265b
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| verdaccioProcess = spawn( | ||
| 'npx', | ||
| [ | ||
| 'verdaccio', | ||
| '--config', | ||
| '.verdaccio/config.yml', | ||
| '--listen', | ||
| `${listenAddress}:${port}`, | ||
| ], | ||
| { stdio: 'ignore', detached: true } | ||
| ); |
There was a problem hiding this comment.
Spawning verdaccio with detached: true without calling unref() will prevent the parent process from exiting until the child exits, defeating the purpose of background execution. Additionally, calling kill() on line 67 may not properly terminate a detached process.
Fix by either:
- Removing
detached: trueif the process should be tied to parent lifecycle - Or calling
verdaccioProcess.unref()after spawn and usingprocess.kill(-verdaccioProcess.pid)to kill the process group:
verdaccioProcess = spawn(..., { stdio: 'ignore', detached: true });
verdaccioProcess.unref();
// In teardown:
if (verdaccioProcess && verdaccioProcess.pid) {
process.kill(-verdaccioProcess.pid);
}Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We believe this failure is an environment artifact ordering issue rather than a defect introduced by the PR. The nx.json cache-bust increment invalidated all cached task outputs, and the CI runner attempted react-native:build-base before @nx/nx had been compiled, leaving the required .d.ts files absent. Re-running the pipeline should resolve this without any code changes.
No code changes were suggested for this issue.
Trigger a rerun:
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
🎓 Learn more about Self-Healing CI on nx.dev
| const interval = setInterval(async () => { | ||
| try { | ||
| const response = await fetch(registryUrl); | ||
| if (response.ok) { | ||
| clearInterval(interval); | ||
| clearTimeout(timeout); | ||
| console.log('Local registry is ready.'); | ||
| resolve(); | ||
| } | ||
| } catch { | ||
| // Registry not up yet | ||
| } | ||
| }, 50); |
There was a problem hiding this comment.
Race condition: Multiple overlapping fetch requests can occur if the async fetch takes longer than 50ms. Each interval tick starts a new async operation without waiting for previous ones to complete. This could spam the registry with requests during startup.
const interval = setInterval(async () => {
try {
const response = await fetch(registryUrl);
if (response.ok) {
clearInterval(interval);
clearTimeout(timeout);
console.log('Local registry is ready.');
resolve();
}
} catch {
// Registry not up yet
}
}, 50);Fix by using a flag to prevent concurrent checks:
let checking = false;
const interval = setInterval(async () => {
if (checking) return;
checking = true;
try {
const response = await fetch(registryUrl);
if (response.ok) {
clearInterval(interval);
clearTimeout(timeout);
console.log('Local registry is ready.');
resolve();
}
} catch {
// Registry not up yet
} finally {
checking = false;
}
}, 50);| const interval = setInterval(async () => { | |
| try { | |
| const response = await fetch(registryUrl); | |
| if (response.ok) { | |
| clearInterval(interval); | |
| clearTimeout(timeout); | |
| console.log('Local registry is ready.'); | |
| resolve(); | |
| } | |
| } catch { | |
| // Registry not up yet | |
| } | |
| }, 50); | |
| let checking = false; | |
| const interval = setInterval(async () => { | |
| if (checking) return; | |
| checking = true; | |
| try { | |
| const response = await fetch(registryUrl); | |
| if (response.ok) { | |
| clearInterval(interval); | |
| clearTimeout(timeout); | |
| console.log('Local registry is ready.'); | |
| resolve(); | |
| } | |
| } catch { | |
| // Registry not up yet | |
| } finally { | |
| checking = false; | |
| } | |
| }, 50); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| const releaseCommand = `pnpm nx-release --local ${publishVersion}`; | ||
| console.log(`> ${releaseCommand}`); |
There was a problem hiding this comment.
Command string injection vulnerability: publishVersion comes from process.env.PUBLISHED_VERSION without validation and is directly interpolated into a shell command. If an attacker controls this environment variable, they can execute arbitrary commands.
const releaseCommand = `pnpm nx-release --local ${publishVersion}`;Fix by passing as a proper argument array or validating the input:
const publishVersion = process.env.PUBLISHED_VERSION ?? 'major';
if (!/^(major|minor|patch|\d+\.\d+\.\d+)$/.test(publishVersion)) {
throw new Error(`Invalid PUBLISHED_VERSION: ${publishVersion}`);
}
const releaseCommand = `pnpm nx-release --local ${publishVersion}`;| const releaseCommand = `pnpm nx-release --local ${publishVersion}`; | |
| console.log(`> ${releaseCommand}`); | |
| const publishVersion = process.env.PUBLISHED_VERSION ?? 'major'; | |
| if (!/^(major|minor|patch|\d+\.\d+\.\d+)$/.test(publishVersion)) { | |
| throw new Error(`Invalid PUBLISHED_VERSION: ${publishVersion}`); | |
| } | |
| const releaseCommand = `pnpm nx-release --local ${publishVersion}`; | |
| console.log(`> ${releaseCommand}`); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
2 participants
Current Behavior
populate-storage.jsandrun-populate-storage.mjsare dead code — env vars set in the script don't propagate to the separatenx-releaseNx taskglobal-setup.tshas a broken import of the deletedrunLocalReleasefunctionnx-releasetarget handles both local (verdaccio) and real (npm) releases, meaning it gets pulled into CI viaaffectedExpected Behavior
populate-local-registry-storagedelegates tonx-releaseviadependsOnnx-releaseis local-only (verdaccio). A newnx-release-npmtarget handles real npm publishes (--local false)publish.ymlusesnx-release-npminstead of callingpnpm nx-releasedirectlynx-release.tswaits for the local registry viawaitForLocalRegistry()before publishingpopulate-local-registry-storagecache is busted to pick up the new task graphRelated Issue(s)
Fixes #34743