-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add .wp-env.json support to wp-env #20002
Conversation
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.
Amazing work here!
I think you need to import types to the files where they are used for them to work.
Is the wp-env reset command useful? It has some problems, namely that it requires wp-env start to have been run at least once. Since we’re bumping the major version here it might be a good chance to replace wp-env-reset with e.g. wp-env destroy which destroys all containers and volumes so that one can run wp-env start from a clean slate. This would be useful for troubleshooting.
We don't have a wp-env reset
but a wp-env destroy
sounds very useful. Perhaps we should add it in a follow-up?
What is the tests-wordpress-phpunit container used for? I could not find where this is used and so have left it out for now.
It was used for PHP unit tests but we held off on it due to flakiness.
@@ -46,31 +46,22 @@ module.exports = function cli() { | |||
yargs.usage( wpPrimary( '$0 <command>' ) ); | |||
|
|||
yargs.command( | |||
'start [ref]', |
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.
This needs a dev note, right?
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.
I noted that this breaks backwards compatibility in packages/env/CHANGELOG.md
but a agree dev note would be good, too. If nothing else it will help publicise that this tool can be used for core development.
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.
👍
I get this error running it locally (in a WordPress plugin with and also without a config file) ~/source/wp-calypso/apps/full-site-editing/full-site-editing-plugin(try/wordpress-env-for-tests) » ~/source/gutenberg/packages/env/bin/wp-env start noah.allen@Noahs-MacBook-Pro
internal/util.js:214
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'original', 'function');
^
TypeError [ERR_INVALID_ARG_TYPE]: The "original" argument must be of type function
at Object.promisify (internal/util.js:214:11)
at Object.<anonymous> (/Users/noah.allen/source/gutenberg/packages/env/lib/detect-directory-type.js:15:23)
at Module._compile (module.js:653:30)
at Object.Module._extensions..js (module.js:664:10)
at Module.load (module.js:566:32)
at tryModuleLoad (module.js:506:12)
at Function.Module._load (module.js:498:3)
at Module.require (module.js:597:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/Users/noah.allen/source/gutenberg/packages/env/lib/config.js:13:29) |
You are running an old node version.
|
Thanks, @epiqueras! Spot on. Turns out I had to |
Glad I could help.
That’s probably due to some custom post install scripts.
|
I'm now getting the following output, which leads me to believe that the custom port number is not working quite right. I'm not sure if it was entirely working before though, as I had similar issues with @wordpress/scripts env with custom port numbers. (Speaking of, it'd be great to add a custom port number to .wp-env.json!) WP_ENV_PORT=3333 ./packages/env/bin/wp-env start
✖ 31911d623e75f345e9ed328b9f48cff6_mysql_1 is up-to-date
Recreating 31911d623e75f345e9ed328b9f48cff6_wordpress_1 ...
Starting 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 ...
Starting 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 ... error
Recreating 31911d623e75f345e9ed328b9f48cff6_wordpress_1 ... done1 (2e501b716d5fdfed3e33bad99337702ef97588c9cabb94003ce8735b5e85d28f): Bind for 0.0.0.0:8889 failed: port is already allocated
ERROR: for tests-wordpress Cannot start service tests-wordpress: driver failed programming external connectivity on endpoint 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 (2e501b716d5fdfed3e33bad99337702ef97588c9cabb94003ce8735b5e85d28f): Bind for 0.0.0.0:8889 failed: port is already allocated
Encountered errors while bringing up the project.
{
exitCode: 1,
err: '31911d623e75f345e9ed328b9f48cff6_mysql_1 is up-to-date\n' +
'Recreating 31911d623e75f345e9ed328b9f48cff6_wordpress_1 ... \r\n' +
'Starting 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 ... \r\n' +
'Host is already in use by another container\n' +
'\u001b[1A\u001b[2K\rStarting 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 ... \u001b[31merror\u001b[0m\r\u001b[1B\n' +
'ERROR: for 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 Cannot start service tests-wordpress: driver failed programming external connectivity on endpoint 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 (2e501b716d5fdfed3e33bad99337702ef97588c9cabb94003ce8735b5e85d28f): Bind for 0.0.0.0:8889 failed: port is already allocated\n' +
'\u001b[2A\u001b[2K\rRecreating 31911d623e75f345e9ed328b9f48cff6_wordpress_1 ... \u001b[32mdone\u001b[0m\r\u001b[2B\n' +
'ERROR: for tests-wordpress Cannot start service tests-wordpress: driver failed programming external connectivity on endpoint 31911d623e75f345e9ed328b9f48cff6_tests-wordpress_1 (2e501b716d5fdfed3e33bad99337702ef97588c9cabb94003ce8735b5e85d28f): Bind for 0.0.0.0:8889 failed: port is already allocated\n' +
'Encountered errors while bringing up the project.\n',
out: ''
} To remedy
If I then try to add a separate instance with a different port number, it also fails. So it seems that even if you specify a custom port number, something under the hood still allocates the default port (8889) for use by the original container. And then when you try to do a second wp-env instance, it thinks that 8889 has been used by something, so it also fails. Here's the output after 1. stopping and removing all docker images. 2. creating a wp-env environment with the custom port 3333. 3. Running the command again in a different plugin with a different port number (9999):
|
You are not specifying a custom port for the tests container so that one
conflicts across environment installs.
|
81da692
to
291aecd
Compare
Thanks for the quick feedback
👍 done.
Sorry, I meant to say: Is
👍 I'll leave it removed for now then. |
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.
Haha, you got the wrong Enrique.
Sorry, I meant to say: Is wp-env clean useful? It has some shortfalls, and since I'm bumping the major version here, it's maybe a good opportunity to remove wp-env clean in lieu of something like wp-env destroy.
I see clean being used to reset a testing environment and destroy being used when things break as a last troubleshooting resort so both can be useful.
I think this is good to merge now. Its a kind of PR that needs a lot of testing in the wild and we can always iterate before the next release. The foundation is strong, thanks for the work!
I think I've done this a dozen times now 😂 |
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.
This is looking really awesome! I hope the issue I noted isn't from yet another node problem :)
I was just thinking about this in the context of @wordpress/scripts. Since we no longer automatically link the directory from which you run the script, we can't really tell what the "main" plugin is for the environment anymore. Perhaps this won't be an issue in most cases, but if I want to run phpunit/phpcs on just my own plugin (perhaps in a CI environment), we'd need to be able to specify that somehow.
This doesn't change any of that. When you run |
If you take a look at the generated |
Thanks @tellthemachines for testing this on Windows! 😍 |
Did you consider using Docker's (and Docker-compose's)'s native support for "env files"? I would suggest defining defaults in one file and then letting optional file (or environment constants!) to overrun that. That might solve issues with Linux and furthermore, be more familiar with anyone already used to Docker setups. Less code at our end, too so easier to maintain. Less our custom-layer specific issues (you'd be still left with Docker & Docker compose specific issues of course). The file format would be different but that's more or less subjective if it's worse or better. Wider support for ways to input values (via environment variables & CLI arguments) is a great bonus.
Thoughts? We have an example setup of something like this in Jetpack: |
Thanks @simison for the suggestion! I wasn't aware of (or had forgotten about) Docker env files, but if they (maybe together with a static Generally, if there's an existing tool and config format, I think it's preferrable to use that, rather than defining a new format. Relying on an existing format means
|
This goes against the goal of The Linux bug is simply a lack of permissions to write in the user folder. |
We did briefly discuss adding support for environment variable support here: #20158 (comment). You can see some more reasoning for why we want to expose a minimal amount of configuration. Essentially, the goal is to be able to run Additionally, if you want to use
There would be nothing stopping anyone from setting up their WordPress configuration in this way. :) It isn't even that much work; the basic config is pretty lightweight. If you want to use docker-compose for lots of power 💪 , that's absolutely fine. But that's not quite the goal of wp-env. If you don't want to use I will say that wp-env gives us some stuff that docker compose does not:
Back to the original issue (filesystems permissions in Linux), I wonder if we could allow users to specify the download location for wp-env source via an environment variable. If set, then wp-env would use that to download sourcecode and configurations to a different location. It seems like the type of setting that would be valuable "per user" |
100% agreed.
|
? process.env.WP_ENV_PORT || '8888' | ||
: process.env.WP_ENV_TESTS_PORT || '8889' | ||
} | ||
--title='${ config.name }' |
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.
I believe the single quotes here are interpreted verbatim by WP-CLI and are causing the Site Title of the default installation to incorrectly include them:
Depending whether it needs to be quoted to account for spaces in the name, it may need double quotes instead to work as expected?
--title='${ config.name }' | |
--title="${ config.name }" |
--title='${ config.name }' | |
--title=${ config.name } |
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.
Thanks! Fixed in #20520.
Hey I would to report there is an error install `sudo npm install -g @wordpress/env
[nodegit] Running pre-install script npm ERR! A complete log of this run can be found in: |
@mewt It's best to create a new issue than to comment on a merged pull request, as an issue would be easier to track. In your case it appears it could be a permission issue stemming from how the internal You shouldn't need nor want to use |
Adds support to
wp-env
for.wp-env.json
files as described in #17724 (comment).Specification
.wp-env.json
supports three fields:"core"
string
ornull
null
null
is specified,wp-env
will use the latest production release of WordPress."plugins"
string[]
[]
"themes"
string[]
[]
Several types of strings can be passed into these fields:
.<path>
or~<path>
"./a/directory"
,"../a/directory"
,"~/a/directory"
/<path>
or<letter>:\<path>
"/a/directory"
,"C:\\a\\directory"
<owner>/<repo>[#<ref>]
"WordPress/WordPress"
,"WordPress/gutenberg#master"
Remote sources will be downloaded into a temporary directory located in
~/.wp-env
.Examples
Latest production WordPress + current directory as a plugin
This is useful for plugin development.
Latest development WordPress + current directory as a plugin
This is useful for plugin development when upstream Core changes need to be tested.
Local
wordpress-develop
+ current directory as a pluginThis is useful for working on plugins and WordPress Core at the same time.
A complete testing environment
This is useful for integration testing: that is, testing how old versions of WordPress and different combinations of plugins and themes impact each other.
Testing
cd
to a directory that contains a WordPress installation, a plugin, a theme, or a.wp-env.json
file/path/to/gutenberg/packages/env/bin/wp-env start
Questions
wp-env reset
command useful? It has some problems, namely that it requireswp-env start
to have been run at least once. Since we’re bumping the major version here it might be a good chance to replacewp-env-reset
with e.g.wp-env destroy
which destroys all containers and volumes so that one can runwp-env start
from a clean slate. This would be useful for troubleshooting.tests-wordpress-phpunit
container used for? I could not find where this is used and so have left it out for now.