-
Notifications
You must be signed in to change notification settings - Fork 2.7k
chore(repo): remove populate-storage scripts, consolidate into nx-release #34814
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
base: master
Are you sure you want to change the base?
Changes from all commits
b9ad989
74622cf
592265b
94a6464
6933b37
cd7c0e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,9 @@ | ||||||||||||||||||
| import { Config } from '@jest/types'; | ||||||||||||||||||
| import { existsSync, removeSync } from 'fs-extra'; | ||||||||||||||||||
| import * as isCI from 'is-ci'; | ||||||||||||||||||
| import { exec, execSync } from 'node:child_process'; | ||||||||||||||||||
| import { ChildProcess, exec, execSync, spawn } from 'node:child_process'; | ||||||||||||||||||
| import { join } from 'node:path'; | ||||||||||||||||||
| import { registerTsConfigPaths } from '../../packages/nx/src/plugins/js/utils/register'; | ||||||||||||||||||
| import { runLocalRelease } from '../../scripts/local-registry/populate-storage'; | ||||||||||||||||||
|
|
||||||||||||||||||
| export default async function (globalConfig: Config.ConfigGlobals) { | ||||||||||||||||||
| try { | ||||||||||||||||||
|
|
@@ -25,14 +24,23 @@ export default async function (globalConfig: Config.ConfigGlobals) { | |||||||||||||||||
| const registry = `http://${listenAddress}:${port}`; | ||||||||||||||||||
| const authToken = 'secretVerdaccioToken'; | ||||||||||||||||||
|
|
||||||||||||||||||
| while (true) { | ||||||||||||||||||
| await new Promise((resolve) => setTimeout(resolve, 250)); | ||||||||||||||||||
| try { | ||||||||||||||||||
| await assertLocalRegistryIsRunning(registry); | ||||||||||||||||||
| break; | ||||||||||||||||||
| } catch { | ||||||||||||||||||
| console.log(`Waiting for Local registry to start on ${registry}...`); | ||||||||||||||||||
| } | ||||||||||||||||||
| // When running outside of Nx (e.g. Jest directly), start verdaccio ourselves | ||||||||||||||||||
| let verdaccioProcess: ChildProcess | undefined; | ||||||||||||||||||
| if (requiresLocalRelease && !(await isLocalRegistryRunning(registry))) { | ||||||||||||||||||
| console.log( | ||||||||||||||||||
| `Local registry not detected at ${registry}, starting verdaccio...` | ||||||||||||||||||
| ); | ||||||||||||||||||
| verdaccioProcess = spawn( | ||||||||||||||||||
| 'npx', | ||||||||||||||||||
| [ | ||||||||||||||||||
| 'verdaccio', | ||||||||||||||||||
| '--config', | ||||||||||||||||||
| '.verdaccio/config.yml', | ||||||||||||||||||
| '--listen', | ||||||||||||||||||
| `${listenAddress}:${port}`, | ||||||||||||||||||
| ], | ||||||||||||||||||
| { stdio: 'ignore' } | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| process.env.npm_config_registry = registry; | ||||||||||||||||||
|
|
@@ -54,6 +62,11 @@ export default async function (globalConfig: Config.ConfigGlobals) { | |||||||||||||||||
| global.e2eTeardown = () => { | ||||||||||||||||||
| // Clean up environment variable instead of npm config command | ||||||||||||||||||
| delete process.env[`npm_config_//${listenAddress}:${port}/:_authToken`]; | ||||||||||||||||||
| // Kill verdaccio if we started it | ||||||||||||||||||
| if (verdaccioProcess) { | ||||||||||||||||||
| verdaccioProcess.kill(); | ||||||||||||||||||
| verdaccioProcess = undefined; | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
|
|
@@ -77,8 +90,26 @@ export default async function (globalConfig: Config.ConfigGlobals) { | |||||||||||||||||
| if (requiresLocalRelease) { | ||||||||||||||||||
| console.log('Publishing packages to local registry'); | ||||||||||||||||||
| const publishVersion = process.env.PUBLISHED_VERSION ?? 'major'; | ||||||||||||||||||
| // Always show full release logs on CI, they should only happen once via e2e-ci | ||||||||||||||||||
| await runLocalRelease(publishVersion, isCI || isVerbose); | ||||||||||||||||||
| const verbose = isCI || isVerbose; | ||||||||||||||||||
| const releaseCommand = `pnpm nx-release --local ${publishVersion}`; | ||||||||||||||||||
| console.log(`> ${releaseCommand}`); | ||||||||||||||||||
|
Comment on lines
+94
to
+95
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Command string injection vulnerability: 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}`;
Suggested change
Spotted by Graphite |
||||||||||||||||||
| await new Promise<void>((resolve, reject) => { | ||||||||||||||||||
| const child = exec(releaseCommand, { | ||||||||||||||||||
| maxBuffer: 1024 * 1000000, | ||||||||||||||||||
| windowsHide: false, | ||||||||||||||||||
| }); | ||||||||||||||||||
| if (verbose) { | ||||||||||||||||||
| child.stdout?.pipe(process.stdout); | ||||||||||||||||||
| child.stderr?.pipe(process.stderr); | ||||||||||||||||||
| } | ||||||||||||||||||
| child.on('exit', (code) => { | ||||||||||||||||||
| if (code === 0) { | ||||||||||||||||||
| resolve(); | ||||||||||||||||||
| } else { | ||||||||||||||||||
| reject(new Error(`Local release failed with exit code ${code}`)); | ||||||||||||||||||
| } | ||||||||||||||||||
| }); | ||||||||||||||||||
| }); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } catch (err) { | ||||||||||||||||||
|
|
@@ -112,9 +143,11 @@ function getPublishedVersion(): Promise<string | undefined> { | |||||||||||||||||
| }); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| async function assertLocalRegistryIsRunning(url) { | ||||||||||||||||||
| const response = await fetch(url); | ||||||||||||||||||
| if (!response.ok) { | ||||||||||||||||||
| throw new Error(`HTTP error! status: ${response.status}`); | ||||||||||||||||||
| async function isLocalRegistryRunning(url: string): Promise<boolean> { | ||||||||||||||||||
| try { | ||||||||||||||||||
| const response = await fetch(url); | ||||||||||||||||||
| return response.ok; | ||||||||||||||||||
| } catch { | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -218,6 +218,13 @@ const VALID_AUTHORS_FOR_LATEST = [ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hackFixForDevkitPeerDependencies(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (options.local) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const port = process.env.NX_LOCAL_REGISTRY_PORT ?? '4873'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const localRegistryUrl = `http://localhost:${port}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await waitForLocalRegistry(localRegistryUrl); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| process.env.npm_config_registry = localRegistryUrl; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Run with dynamic output-style so that we have more minimal logs by default but still always see errors | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let publishCommand = `pnpm nx release publish --registry=${getRegistry()} --tag=${distTag} --output-style=dynamic --parallel=8`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (options.dryRun) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -400,10 +407,6 @@ function parseArgs() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'Registry is still set to localhost! Run "pnpm local-registry disable" or pass --force' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!args.force && !registryIsLocalhost) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error('--local was passed and registry is not localhost'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -476,6 +479,33 @@ function determineDistTag( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return distTag; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function waitForLocalRegistry(registryUrl: string): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log(`Waiting for local registry at ${registryUrl}...`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new Promise<void>((resolve, reject) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const timeout = setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clearInterval(interval); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| reject( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new Error( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `Local registry at ${registryUrl} did not become available within 60 seconds` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, 60_000); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+493
to
+505
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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);
Suggested change
Spotted by Graphite |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //TODO(@Coly010): Remove this after fixing up the release peer dep handling | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function hackFixForDevkitPeerDependencies() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { readFileSync, writeFileSync } = require('fs'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Spawning verdaccio with
detached: truewithout callingunref()will prevent the parent process from exiting until the child exits, defeating the purpose of background execution. Additionally, callingkill()on line 67 may not properly terminate a detached process.Fix by either:
detached: trueif the process should be tied to parent lifecycleverdaccioProcess.unref()after spawn and usingprocess.kill(-verdaccioProcess.pid)to kill the process group:Spotted by Graphite

Is this helpful? React 👍 or 👎 to let us know.