Skip to content

Commit bd4378b

Browse files
CGastrellclaude
andauthored
Docker CLI: scope parallel .env handling to type=dev, exclude e2e (#49160)
The hybrid tools/docker/.env machinery added in #48643 (read parallel keys as a base layer, conflict warnings, append-on-`up --name` persistence) gated its read block on `up` and its persist block on `up --name`, but neither checked `argv.type === 'dev'`. The E2E framework runs `jetpack docker --type e2e --name t1 up -d` (tools/e2e-commons/bin/e2e-env.sh), which sets argv.name. So running the E2Es in a plain checkout wrote COMPOSE_PROJECT_NAME=jetpack_t1 and the PORT_* keys into tools/docker/.env — reported in #48643 (comment). That pollution cascades: a subsequent bare `jetpack docker up` (type=dev) reads the polluted .env, infers argv.name='t1' via augmentArgvFromEnvFile, and silently brings the dev container up as jetpack_t1 on the E2E ports. Fix: add an exported `shouldManageParallelEnv( argv )` predicate (`type === 'dev' && _[1] === 'up'`) and gate both the read/conflict block and the persist block through it — mirroring resolveDevCloneSource being "correct on its own terms". E2E now neither reads parallel keys from nor writes them to the shared .env, and uses its own 8889 default port. The buildEnv/process-env path is untouched, so E2E's own container behavior is unchanged. Adds a shouldManageParallelEnv describe block (4 cases incl. the e2e regression). tools/cli suite: 156 passed, 3 skipped. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 74e17f8 commit bd4378b

2 files changed

Lines changed: 51 additions & 2 deletions

File tree

tools/cli/commands/docker.js

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,21 @@ export const applyUpdateEnv = ( filePath, conflicts ) => {
343343
return updated;
344344
};
345345

346+
/**
347+
* Whether the hybrid tools/docker/.env parallel-instance machinery (reading keys as a base
348+
* layer, conflict warnings, append-on-`up --name` persistence) applies to this invocation.
349+
*
350+
* Scoped to `up` on `type === 'dev'`. The e2e flow runs `docker --type e2e --name t1 up`,
351+
* which sets argv.name but manages its own fixed ports and project name — it must neither
352+
* read parallel keys from nor write them to the shared .env. Keeping the gate here (rather
353+
* than only inline at the call sites) mirrors resolveDevCloneSource being "correct on its
354+
* own terms" and makes the e2e exclusion unit-testable.
355+
*
356+
* @param {object} argv - Yargs argv.
357+
* @return {boolean} true when parallel-env reads/writes should run.
358+
*/
359+
export const shouldManageParallelEnv = argv => argv.type === 'dev' && argv._[ 1 ] === 'up';
360+
346361
/**
347362
* Decides which source instance to clone the DB from when bringing up a dev container, if any.
348363
*
@@ -810,7 +825,10 @@ const defaultDockerCmdHandler = async argv => {
810825
// surface conflicts as warnings (or rewrite when --update-env is set), and persist
811826
// parallel-instance keys after a successful `up --name`. See tools/docker/README.md
812827
// § "Parallel development environments" for the full precedence + semantics.
813-
if ( argv._[ 1 ] === 'up' ) {
828+
// Scoped via shouldManageParallelEnv (`up` + `type === 'dev'`): the e2e flow runs with a
829+
// fixed `--name t1` and manages its own ports, so it must neither read parallel keys from
830+
// nor write them to the shared tools/docker/.env.
831+
if ( shouldManageParallelEnv( argv ) ) {
814832
const flagSnapshot = snapshotFlagArgv( argv );
815833
const fileEnv = readEnvFile();
816834
augmentArgvFromEnvFile( argv, fileEnv );
@@ -901,7 +919,9 @@ const defaultDockerCmdHandler = async argv => {
901919
// Persist parallel-instance config to .env on `up --name`. Idempotent: only appends
902920
// keys that aren't already in the file (Strategy B). Skipped for the primary
903921
// `jetpack_dev` flow (no --name) so the main checkout's .env is never written to.
904-
if ( argv._[ 1 ] === 'up' && argv.name ) {
922+
// Also skipped for non-dev types: the e2e flow always passes `--name t1`, but it
923+
// must not write COMPOSE_PROJECT_NAME/PORT_* into the shared tools/docker/.env.
924+
if ( shouldManageParallelEnv( argv ) && argv.name ) {
905925
const written = persistParallelEnv( envOpts );
906926
if ( written.length ) {
907927
console.log(

tools/cli/tests/unit/commands/docker.test.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ const {
3737
detectEnvConflicts,
3838
persistParallelEnv,
3939
applyUpdateEnv,
40+
shouldManageParallelEnv,
4041
PARALLEL_ENV_KEYS,
4142
} = await import( '../../../commands/docker.js' );
4243

@@ -197,6 +198,34 @@ describe( 'resolveDevCloneSource', () => {
197198
} );
198199
} );
199200

201+
describe( 'shouldManageParallelEnv', () => {
202+
test( 'true for `up` on the default dev type', () => {
203+
expect( shouldManageParallelEnv( { type: 'dev', _: [ 'docker', 'up' ] } ) ).toBe( true );
204+
} );
205+
206+
test( 'true for `up --name` on dev', () => {
207+
expect(
208+
shouldManageParallelEnv( { type: 'dev', name: 'feature', _: [ 'docker', 'up' ] } )
209+
).toBe( true );
210+
} );
211+
212+
// Regression: the e2e framework runs `docker --type e2e --name t1 up -d`. Before the
213+
// type gate, that --name leaked COMPOSE_PROJECT_NAME/PORT_* into the shared
214+
// tools/docker/.env of a plain checkout. See PR #48643 review follow-up.
215+
test( 'false for the e2e flow (--type e2e --name t1 up)', () => {
216+
expect( shouldManageParallelEnv( { type: 'e2e', name: 't1', _: [ 'docker', 'up' ] } ) ).toBe(
217+
false
218+
);
219+
} );
220+
221+
test( 'false for non-up commands on dev', () => {
222+
expect( shouldManageParallelEnv( { type: 'dev', _: [ 'docker', 'down' ] } ) ).toBe( false );
223+
expect(
224+
shouldManageParallelEnv( { type: 'dev', name: 'feature', _: [ 'docker', 'clean' ] } )
225+
).toBe( false );
226+
} );
227+
} );
228+
200229
describe( 'normalizeProjectShortName', () => {
201230
test( 'lowercases mixed-case input', () => {
202231
expect( normalizeProjectShortName( 'Feature' ) ).toBe( 'feature' );

0 commit comments

Comments
 (0)