-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Build/Test Tools: Improve dev environment's CLI in speed, non-interactive usage, and argument handling #8969
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
Changes from 9 commits
a3cf770
23ac059
6b3de67
7a66e62
b8b1425
f4d6575
bcbf5da
3e2fa2e
8f2af97
0e4ddbb
12b4457
1ce3750
ec71151
45954e9
1b2ef30
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 |
|---|---|---|
|
|
@@ -111,6 +111,9 @@ services: | |
| volumes: | ||
| - ./:/var/www | ||
|
|
||
| # Keeps the service alive. | ||
| command: 'sleep infinity' | ||
|
Member
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. For a true idling container I would rather use
Member
Author
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. I chose this for consistency with wp-env.
Member
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. It seems there were no big explanations
Member
Author
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. If Gutenberg switches back to |
||
|
|
||
| # The init directive ensures the command runs with a PID > 1, so Ctrl+C works correctly. | ||
| init: true | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,7 +184,7 @@ | |
| "env:clean": "node ./tools/local-env/scripts/docker.js down -v --remove-orphans", | ||
| "env:reset": "node ./tools/local-env/scripts/docker.js down --rmi all -v --remove-orphans", | ||
| "env:install": "node ./tools/local-env/scripts/install.js", | ||
| "env:cli": "node ./tools/local-env/scripts/docker.js run --rm cli", | ||
| "env:cli": "node ./tools/local-env/scripts/docker.js exec cli wp --allow-root", | ||
|
Member
Author
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. Why And the entrypoint is using |
||
| "env:logs": "node ./tools/local-env/scripts/docker.js logs", | ||
| "env:pull": "node ./tools/local-env/scripts/docker.js pull", | ||
| "test:performance": "wp-scripts test-playwright --config tests/performance/playwright.config.js", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,36 @@ | ||
| const dotenv = require( 'dotenv' ); | ||
| /* jshint node:true */ | ||
|
|
||
| const dotenv = require( 'dotenv' ); | ||
| const dotenvExpand = require( 'dotenv-expand' ); | ||
| const { execSync } = require( 'child_process' ); | ||
| const { spawnSync } = require( 'child_process' ); | ||
| const local_env_utils = require( './utils' ); | ||
|
|
||
| dotenvExpand.expand( dotenv.config() ); | ||
|
|
||
| const composeFiles = local_env_utils.get_compose_files(); | ||
|
|
||
| if (process.argv.includes('--coverage-html')) { | ||
| if ( process.argv.includes( '--coverage-html' ) ) { | ||
| process.env.LOCAL_PHP_XDEBUG = 'true'; | ||
| process.env.LOCAL_PHP_XDEBUG_MODE = 'coverage'; | ||
| } | ||
|
|
||
| // This try-catch prevents the superfluous Node.js debugging information from being shown if the command fails. | ||
| try { | ||
| // Execute any Docker compose command passed to this script. | ||
| execSync( 'docker compose ' + composeFiles + ' ' + process.argv.slice( 2 ).join( ' ' ), { stdio: 'inherit' } ); | ||
| } catch ( error ) { | ||
| process.exit( 1 ); | ||
| // Add --no-TTY (-T) arg after exec and run commands when STDIN is not a TTY. | ||
| const dockerCommand = process.argv.slice( 2 ); | ||
| if ( [ 'exec', 'run' ].includes( dockerCommand[0] ) && ! process.stdin.isTTY ) { | ||
| dockerCommand.splice( 1, 0, '--no-TTY' ); | ||
| } | ||
|
|
||
| // Execute any Docker compose command passed to this script. | ||
| const returns = spawnSync( | ||
| 'docker', | ||
| [ | ||
| 'compose', | ||
| ...composeFiles | ||
| .map( ( composeFile ) => [ '-f', composeFile ] ) | ||
| .flat(), | ||
| ...dockerCommand, | ||
| ], | ||
| { stdio: 'inherit' } | ||
| ); | ||
|
|
||
| process.exit( returns.status ); |
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.
The
wpis removed here because previously the entrypoint would inject it if it was missing: https://github.com/WordPress/wpdev-docker-images/blob/becc6740684d6ff6414d6539df040633b5ac5794/entrypoint/entrypoint-cli.sh#L16-L17With this PR, the
wpis always assumed.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.
Since this is technically a breaking change, I searched github for
npm run env:cli wpand there is only one reference which seems to be from a fork of a demo rather than actual production code, but it still might be worth a PR there after this lands and a quick note in the core room so folks are aware. I don't think it needs a dev note though since this isn't a public API.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.
PR opened: Antonin06/wc-bretagne-2024#1