-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 all 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' | ||
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 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. 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 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", | ||
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 ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,10 @@ dotenvExpand.expand( dotenv.config() ); | |
local_env_utils.determine_auth_option(); | ||
|
||
// Create wp-config.php. | ||
wp_cli( `config create --dbname=wordpress_develop --dbuser=root --dbpass=password --dbhost=mysql --force --config-file=${process.env.LOCAL_DIR}/../wp-config.php` ); | ||
wp_cli( `config create --dbname=wordpress_develop --dbuser=root --dbpass=password --dbhost=mysql --force --config-file="wp-config.php"` ); | ||
|
||
// Since WP-CLI runs as root, the wp-config.php created above will be read-only. This needs to be writable for the sake of E2E tests. | ||
execSync( 'node ./tools/local-env/scripts/docker.js exec cli chmod 666 wp-config.php' ); | ||
Comment on lines
+18
to
+19
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's not clear to me why this is needed now. Without it, E2E tests fail due to In 0e4ddbb I tried using the node API to set the file permissions, but this failed because it's not being run as root: https://github.com/WordPress/wordpress-develop/actions/runs/15624512371/job/44016093197 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 think in 0e4ddbb it was running on the local filesystem whereas in this implementation it is running in the container. |
||
|
||
// Add the debug settings to wp-config.php. | ||
// Windows requires this to be done as an additional step, rather than using the --extra-php option in the previous step. | ||
|
@@ -61,7 +64,5 @@ wait_on( { | |
* @param {string} cmd The WP-CLI command to run. | ||
*/ | ||
function wp_cli( cmd ) { | ||
const composeFiles = local_env_utils.get_compose_files(); | ||
|
||
execSync( `docker compose ${composeFiles} run --quiet-pull --rm cli ${cmd} --path=/var/www/${process.env.LOCAL_DIR}`, { stdio: 'inherit' } ); | ||
execSync( `npm --silent run env:cli -- ${cmd} --path=/var/www/${process.env.LOCAL_DIR}`, { stdio: 'inherit' } ); | ||
} |
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
wp
is 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
wp
is 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 wp
and 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