Docker CLI parallel flags: PR review follow-up (#48153)#48643
Open
CGastrell wants to merge 15 commits into
Open
Docker CLI parallel flags: PR review follow-up (#48153)#48643CGastrell wants to merge 15 commits into
CGastrell wants to merge 15 commits into
Conversation
Honor --name and --port for the dev container type, add --port-phpmy / --port-inbox / --port-smtp / --port-sftp so the aux services can be moved off default ports, drop the hardcoded mailpit container_name that prevented a second instance from coming up, and fix the post-up URL so it prints localhost:PORT rather than localhostPORT. Enables running two worktrees with isolated Docker environments side by side.
Passes HOST_PORT through wp core install --url so non-default-port instances get the right siteurl/home in their DB, and fixes the 'Your site is ready' message that previously referenced an unset PORT_WORDPRESS.
Introduces .agents/skills/work-on.md, a team-shared skill that takes a task prompt, plans, spawns an isolated worktree + jp docker instance via the new parallel-environment flags, implements, runs build/test/phan gates, captures before/after screenshots for visual changes, creates a changelog entry when needed, and opens a draft PR. Three modes (full, bootstrap-only, implement-only) and deterministic port-band allocation so multiple collaborators can share the approach without collisions.
…cess signals Rewrites the description with trigger phrases so the skill actually fires when a user says "work on", "ship", "implement", etc. Adds a concrete walkthrough example, a pinned .work-on/env.json schema for Mode 3 resume, an explicit success-signals block to make completion measurable, a failure-path cleanup step, a pnpm-install fallback, and a prerequisites section clarifying jp / jetpack / pnpm jetpack equivalence. Extracts port-band allocation and worktree bootstrap into work-on/scripts/ so the deterministic bits stop being re-derived every run.
When a second (or third) Docker instance is brought up with --name, clone the database from the running jetpack_dev instance so the parallel site is immediately usable — no separate install or Jetpack reconnection needed. Triggers: --name set + jetpack_dev running + target not already installed. Opt-out: --no-clone. Explicit source: --clone-from <name>. Guarded against cloning onto oneself and against re-cloning an already-installed target. Adds resolveCloneSource() (pure, tested), an isProjectRunning() docker ps probe, and cloneDatabase() that waits for the target to be ready, pipes `wp db export` into `wp db import`, then search-replaces the source siteurl to the target's localhost:<port>. Also extends the docker README with a "Parallel development environments" section documenting the flags and cleanup commands.
Co-authored-by: enejb <115071+enejb@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Automattic/jetpack/sessions/29d4a544-0f43-43eb-98c7-01d5226ec298 Co-authored-by: enejb <115071+enejb@users.noreply.github.com>
Compose project names must be lowercase; passing `--name Feature` previously blew up at compose level with a terse error. Now the CLI normalizes the short name (lowercase) via a yargs coerce and prints a one-line yellow notice when it had to change the input, so mixed-case names "just work" for devs. Invalid characters that aren't a case issue (spaces, slashes, dots, leading dashes/underscores) still fail, but now with a clear message up-front rather than from compose. Same treatment for `--clone-from`. Adds a pure `normalizeProjectShortName()` helper plus four tests covering lowercase, pass-through, invalid-char rejection, and leading-char rejection.
The prior wording said "default source is `dev` when the flag is passed without a value" — misleading. `--clone-from` is a string option that requires a value; there is no "passed without a value" mode. The `jetpack_dev` default applies only when the flag is omitted entirely (auto-clone path). Rephrase to separate the two code paths and mention --no-clone.
alloc-ports.sh: switch from scanning docker-forwarded ports to lsof, which also sees host-bound listeners. Addresses anomiex's point that 8080 is commonly in use on a12s' machines outside docker — the old check missed those and would hand out a colliding band. docker.js: drop the `argv.detached` gate on auto-clone. Instead, check `isProjectRunning(target)` right before cloning. In foreground `jetpack docker up`, composeExecutor blocks until the user exits compose, at which point the target is stopped — now the clone skips silently instead of refusing to run based on a flag.
So a future agent reading AGENTS.md for the canonical docker commands finds the parallel-instance flag set and the /work-on skill, instead of defaulting to single-instance `jp docker up` and missing the option.
Rename resolveCloneSource → resolveDevCloneSource and move the type === 'dev' gate from the caller into the function body, so the function is correct on its own terms regardless of where it's called from. The caller's outer 'argv.type === dev' check is dropped (now redundant); 'argv._[1] === up' stays. Addresses PR review feedback from @anomiex on #48153 (comment) > It probably also doesn't make sense for --type=e2e. The E2E > framework probably wants to populate the database instead. The original implementation kept resolveCloneSource purely argv-driven and applied the dev-only gate at the caller. That's fine in practice but leaves the function semantically wrong: given { type: 'e2e', name: 'foo', clone: true } it returned a non-null clone target, requiring every future caller to remember the type gate. Naming + internal guard fix that. Test added: resolveDevCloneSource({type:'e2e', ...}) returns null regardless of other flags.
Replace `bash -c 'docker exec … wp db export | docker exec -i … wp db import'` with a node-native `spawn` pair piped via streams. Each side now has its own exit code and is awaited independently, so failures are attributed correctly instead of being masked by the last-command-wins semantics of an unguarded shell pipe. Addresses PR review feedback from @anomiex on #48153 (comment) > Meh. This probably works, but usually we do the process plumbing > inside node with its `.pipe()` method on streams. The reply we left on the PR defended the bash form on functional grounds (kernel-buffered pipe, no node-side memory pressure for a full DB dump). That's true but missed the actual concern: the bash invocation we shipped did NOT set `pipefail`, so a `wp db export` that errored would silently produce partial/empty stdout, and an importer that happened to consume it without erroring would let the pipe report exit 0 — "Clone complete" with a half-cloned DB. The node-native rewrite makes that bug structurally impossible: each spawn returns its own exit code, and pipeDbDump rejects with source/target attribution as appropriate. Bonus: removes the only `bash` dependency in this file, aligning with the surrounding code's spawn-based style and incidentally making the call portable to any node-supported platform. Tests added (4): both-pass, source-fails (the silent-failure scenario the bash form masked), target-fails, both-fail with combined attribution. Mocking is via a fresh `child_process` module mock + a small `makeMockProc` helper that returns EventEmitter-shaped processes with controllable exit codes.
Add a hybrid config plane: the CLI now reads tools/docker/.env as a base layer for parallel-instance values, flags still override per invocation, and "up --name <slug>" appends the resolved values to .env so subsequent bare "jp docker up" from the same worktree picks them up. Conflicts between flags and .env produce a one-line warning by default; pass --update-env to rewrite the conflicting key(s) in place. Addresses PR review feedback from @anomiex on #48153 (comment) > I wonder, instead of adding all these options, if it would make > more sense to write the environment variables to > tools/docker/.env in your new worktree directory. Also resolves the latent post-up URL bug raised at #48153 (comment) > I note the message is still wrong if tools/docker/.env was > used instead... Reading .env as the base layer for argv ensures printPostCmdMsg gets a populated argv.port whether the user passed --port or set PORT_WORDPRESS in .env, so the post-up URL is now accurate either way. The original reply on the PR enumerated four reservations and offered the persist-on-first-up middle ground as a follow-up. This commit acts on that middle ground in this PR. Worktrees already make .env per-directory; making the CLI flag-aware AND .env-aware preserves the scriptable invocation we use in /work-on while giving the manual workflow the ergonomics anomiex was asking for. Design choices (locked after a Q&A walkthrough): * Strategy B append-only: persistParallelEnv parses the existing .env, appends only the parallel keys not already present. It never modifies, reorders, or rewrites pre-existing lines. Comments and user-managed keys (WP_ADMIN_USER, etc.) are preserved verbatim. * Read precedence on "up": default.env -> .env -> flags -> process.env. * Persist gate: only when --name is set (i.e. parallel-instance intent). The primary jetpack_dev flow never writes to .env. * COMPOSE_PROJECT_NAME -> argv.name inference is gated against jetpack_dev / jetpack_e2e to prevent the main checkout's .env from silently redirecting the dev container. * Conflict policy: warn (don't prompt -- would break /work-on and any non-interactive use). --update-env is the explicit opt-in for rewriting .env, keeping the principle that the CLI never silently mutates a file the user wrote. New exported helpers (unit-tested): PARALLEL_ENV_KEYS, readEnvFile, snapshotFlagArgv, augmentArgvFromEnvFile, detectEnvConflicts, persistParallelEnv, applyUpdateEnv Docs: tools/docker/README.md gains a "How .env is used" section covering the read precedence, what gets written when, conflict behavior, and a worked example. tools/docker/default.env header gains a comment block flagging the keys the CLI may append. ~30 new unit tests covering the precedence matrix, the Strategy B append-only behavior, the dev/e2e inference guard, multi-conflict aggregation, and applyUpdateEnv's in-place line replacement.
Add a precondition bullet to the auto-clone "only triggered when all of the following are true" list spelling out the foreground-vs-detached behavior: in foreground mode the parallel target stops as soon as compose exits, so the auto-clone has nowhere to write into and is silently skipped. Recommends -d for any persistent parallel instance including the /work-on flow. Addresses PR review feedback from @anomiex on #48153 (comment) > Also, apparently, you passed -d / --detached to jetpack docker up. The behavioral fix landed in 6c4f1fa on the parallel-flags branch (gate on isProjectRunning(target) instead of on argv.detached, so foreground mode no longer hard-errors -- it skips silently). What was missing was the documentation update so a future reader -- human or AI agent -- can pattern-match on -d / --detached and learn the practical consequence without spelunking the source. Wording is intentionally explicit ("If you run up in the foreground... the auto-clone is silently skipped... always pass -d") so an AI agent running the README through grep / retrieval picks up the trigger -> consequence -> prescription cleanly.
github-actions
Bot
added
[Status] In Progress
[Tests] Includes Tests
[Tools] Development CLI
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
github-actions
Bot
added
the
[Status] Needs Author Reply
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #
Proposed changes
Stacked PR responding to the eight review comments on #48153. Targets the original PR branch (
change/docker-cli-parallel-flags) rather thantrunkso it can be held, merged into the parent, or discarded as a unit. Each commit corresponds to one (or two related) review threads; the commit message quotes the reviewer verbatim and links back to the#discussion_r…URL so the rationale travels with the code.Commit-by-thread map
579d27a3e4b--type=e2e. The E2E framework probably wants to populate the database instead."resolveCloneSource→resolveDevCloneSource; moved thetype === 'dev'gate from the caller into the function body so it's correct on its own terms. New unit test assertse2ereturnsnullregardless of other flags.403cb106b29.pipe()method on streams."bash -c '… | …'with a node-nativepipeDbDumphelper using twospawn()calls +source.stdout.pipe(target.stdin). Each side now has its own awaited exit code. Fixes a latent silent-failure bug: the bash invocation didn't setpipefail, so a brokenwp db exportcould be masked by awp db importthat exited 0 on partial input — "Clone complete" with a half-cloned DB. 4 new unit tests cover all four exit-code combinations.ae3dee033b3tools/docker/.envin your new worktree directory." / "I note the message is still wrong iftools/docker/.envwas used instead...".envconfig: CLI readstools/docker/.envas a base layer for parallel-instance keys, flags still override per invocation, andup --name <slug>appends resolved values to.envso subsequent barejp docker upfrom the same worktree just works. New--update-envflag opt-in for rewriting conflicting keys in place; default behavior is a one-line warning. Strategy B append-only persistence: existing.envlines (comments,WP_ADMIN_USER, etc.) are never modified. Thread 6's post-up-URL bug auto-resolves becauseargv.portis now reliably populated whether passed as a flag or read from.env. ~30 new unit tests covering precedence, conflicts, append-only, in-place updates, and thejetpack_dev/jetpack_e2einference guard.a33494bd67d-d/--detachedtojetpack docker up."-d, compose runs in the foreground, the parallel target stops when you exit, and the auto-clone is silently skipped. Wording is intentionally explicit (-d/--detachedkeywords, trigger → consequence → prescription) so an AI agent grepping the README picks it up cleanly. The behavioral half of this thread already landed in6c4f1faon the parent branch (gate onisProjectRunning(target)instead ofargv.detached).Threads not addressed in this PR (already resolved)
--port-*flags already let users handpick every port.lsof-based port detection (@anomiex)bb30b85on the parent branch.argv.detachedgate on auto-clone (@anomiex)6c4f1faon the parent branch.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Quick — unit tests
From
tools/cli:pnpm testExpected: 152 passed, 3 skipped (existing). New coverage spans
resolveDevCloneSourcee2e guard,pipeDbDumpexit-code matrix, the.envprecedence + Strategy B append-only behavior, conflict aggregation, andapplyUpdateEnvline-replacement.Two-instance smoke test (the high-value path)
Confirms the hybrid
.envflow end-to-end. Assumesjetpack_devis up and installed.jetpack_envtest-*containers running. Output ends withPersisted to tools/docker/.env: COMPOSE_PROJECT_NAME, PORT_WORDPRESS, PORT_PHPMY, PORT_INBOX, PORT_SMTP, PORT_SFTP. ….envwas appended (not rewritten):WP_ADMIN_USER, etc.) untouched, followed by a comment block and the six parallel-instance keys.upreads.env:jetpack_envtest-*comes back up on the same ports. No warnings.--update-env):⚠ PORT_WORDPRESS: flag value '8090' differs from .env value '8080'. Using flag for this run; pass --update-env to persist, or drop the flag to use .env.tools/docker/.envunchanged on disk. Site comes up on:8090for this run.--update-envrewrites in place:Updated tools/docker/.env keys to match flags: PORT_WORDPRESS.cat tools/docker/.envshows the line is nowPORT_WORDPRESS=8090; every other line preserved verbatim.pipeDbDumpfailure attribution. Hard to provoke deliberately without breaking your DB; the unit tests cover the four exit-code combinations. The behavior-of-record on success is unchanged:Cloning database: jetpack_dev → jetpack_envtestfollowed byDumping source DB and importing into target…andClone complete. Target site ready at http://localhost:8090/.jetpack_devis still untouched — from your main checkout (NOT the worktree):Teardown
jetpack docker stop --name envtest jetpack docker clean --name envtest # nuke the parallel DB git worktree remove --force ../jetpack-envtestRegression notes
--type=e2eflow is unaffected:resolveDevCloneSourcereturnsnullfor e2e regardless of other flags, so e2e never auto-clones (which it shouldn't — it manages its own DB).bashis no longer a dependency for the clone path; portable to any node-supported platform.process.envanddefault.envprecedence rules unchanged.