Skip to content
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 Post-Install Command #49996

Closed
wants to merge 10 commits into from
Closed

Add wp-env Post-Install Command #49996

wants to merge 10 commits into from

Conversation

ObliviousHarmony
Copy link
Contributor

What?

This pull request will execute a user-defined postInstall command on wp-env start and, optionally, wp-env clean.

Why?

By allowing users to run their own command after installation, they will be able to define setup scripts and the like that perform additional processing on the environment post-setup.

How?

The user can set a postInstall configuration in .wp-env.json that will be executed after configuring WordPress. They can also use a WP_ENV_POST_INSTALL environment variable to override any configured scripts.

Testing Instructions

  1. Create (or edit) a .wp-env.override.json to include:
{
  "postInstall": "echo \"Test\""
}
  1. Run npm run env -- start --update and verify that Test is printed twice.
  2. Run npm run env -- start --update --no-post-install and verify that Test is not printed.
  3. Run WP_ENV_POST_INSTALL="ls -la" npm run env -- start --update and verify that the Gutenberg directory is printed.
  4. Run npm run env -- clean and verify that Test is not printed.
  5. Run npm run env -- clean --post-install and verify that Test is printed.
  6. Change .wp-env.override.json:
{
    "postInstall": "echo \"$WP_ENV_POST_INSTALL_ENVIRONMENT:$WP_ENV_POST_INSTALL_CONTEXT:$WP_ENV_POST_INSTALL_DEBUG\""
}
  1. Run npm run env -- start --update and verify that both development:start:false and tests:start:false are printed.
  2. Run npm run env -- start --update --debug and verify that both development:start:true and tests:start:true are printed.
  3. Change .wp-env.override.json:
{
  "postInstall": "echo 'Test' > /dev/null"
}
  1. Run npm run env -- start --update and verify that there is no output.
  2. Change .wp-env.override.json:
{
  "postInstall": "ech"
}
  1. Run npm run env -- start --update and verify that there is an error.

While at the moment this doesn't do anything,
it does, however, exist. You can also override it
with the `WP_ENV_POST_INSTALL` environment
variable.
This commit adds support for executing the post-install script
when the user runs `wp-env start` and it re-configured WordPress.
This adds an optional flag for executing the post-install
commands when the environment is cleaned.
@noahtallen noahtallen assigned noahtallen and unassigned noahtallen Apr 21, 2023
@noahtallen noahtallen added [Tool] Env /packages/env [Type] Feature New feature to highlight in changelogs. labels Apr 21, 2023
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I have two broader thoughts:

  • IMO, post install could be run by default on wp-env clean.
  • I'm not convinced about having post-install run twice for each environment.

For the latter, I feel like most people aren't going to need a separate script. For example, if my goal is to add data to the test database, I might run wp-env run tests-cli "wp ...". And if I want to add the same for the main database, I'd do wp-env run cli "wp ...". (E.g. I'm still writing that out twice.)

So if I would just write a single post-install script, I wouldn't expect it to run multiple times like this, at least by default.

(A side note is whether there could be race conditions using execSync on the same file at the same)

Happy to be convinced otherwise, but I'm just not sure the ergonomics are that much different having it run twice.

packages/env/CHANGELOG.md Outdated Show resolved Hide resolved
--help Show help [boolean]
--version Show version number [boolean]
--debug Enable debug output. [boolean] [default: false]
--post-install Execute the environments' configured post-install command if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be the default for clean. My reasoning is that we say post install gets run when WordPress is configured, which is also done during the clean step. 🤔 Plus, if someone has post-install configured, a common use case is making sure data exists in the database

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that makes sense. This is an opt-in feature, so, you can operate under that expectation the first time you use it without concern.

packages/env/README.md Outdated Show resolved Hide resolved
packages/env/README.md Outdated Show resolved Hide resolved
@@ -43,6 +43,10 @@ const withSpinner =
// Error is a validation error. That means the user did something wrong.
spinner.fail( error.message );
process.exit( 1 );
} else if ( error instanceof env.PostInstallError ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could be handled in the same case as instance of env.ValidationError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, I don't see a good reason not to do it.

packages/env/lib/wordpress.js Outdated Show resolved Hide resolved
@@ -668,6 +680,26 @@ You can tell `wp-env` to use a specific PHP version for compatibility and testin
}
```

#### Post-Install Command
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to document pointing to your own script! (node or shell)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a little superfluous since you can run arbitrary commands, but, more examples can't hurt!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True! I feel like it could be easy to miss if you aren't super familiar with it

@ObliviousHarmony
Copy link
Contributor Author

ObliviousHarmony commented Apr 21, 2023

Thanks for the feedback @noahtallen!

I'm not convinced about having post-install run twice for each environment.

To be fair, you're only running it twice if you use the top-level configuration option:

  • If you wanted to use a single script and have some special handling, you can use the WP_ENV_POST_INSTALL_ENVIRONMENT environment variable in your script pretty easily. They can also use the same variable to run the same commands on each environment if they want. Lots of choices!
  • You can alternatively use the environment-specific configuration to run it in a single environment. No need for any other complication.

As it stands, Gutenberg's own tests environment contains a significant number of plugins that the development environment does not. It feels aligned with that principle to have a separate postInstall file that addresses any concerns E2E tests may have.

I like the flexibility that this provides when treated like the rest of the configuration options. It would feel strange if this was the only .wp-env.json option that behaved this way. Would this be acceptable if it was documented and we switched out the examples for an environment-specific one against tests? That would push people towards their most-likely use-case while still providing that flexibility without more complication around options and commands.

A side note is whether there could be race conditions using execSync on the same file at the same

Depending on what the script is doing, this may be true. I would wager that most scripts aren't going to be doing anything contentious, since at worst, they would be doing the same thing. Given that the script is supposed to be written with multiple executions in mind, there shouldn't be issues.

@noahtallen
Copy link
Member

Good points. What stands out to me with this option is that it's not actually unique to the environment, which is very different from the other options. For example, when I set the port or WordPress version, those are by definition unique to that environment. It can't possibly change something in the other environment. But with this script, you can do whatever you want! To me, that makes it less of an environment-specific thing, and more of a wp-env-tooling-specific thing. (E.g. "execute this code at this part of the lifecycle")

To me, having an environment level option implies that it will only do something to that environment. But you could easily write your development script to update tests, and your tests script to update development. (Not that that's really a problem -- but to me, that's why it feels different from the other options)

It just makes me wonder what having the environment-level option actually enables users to do:

# Pseodocode for script executed twice:
cmd="wp foo"
if ( WP_ENV_POST_INSTALL_ENVIRONMENT == 'tests' ) {
  npx wp-env tests-cli "$cmd"
} else {
  npx wp-env cli "$cmd"
}
# Script executed once:
cmd="wp foo"
npx wp-env tests-cli "$cmd"
npx wp-env cli "$cmd"

@noahtallen
Copy link
Member

I think I see the value of specifying different scripts (especially for inline values) in the environment configs. But I still don't love that putting it in the root configuration executes it twice. I think folks won't expect that

@ObliviousHarmony
Copy link
Contributor Author

My biggest concern here is consistency @noahtallen, but, I think you might be right. When you set a top-level "core", for example, it is singular conceptually because it's set once (but in both containers). It's possible that, on that principle, it might make sense to only run it a single time.

I acquiesce :)

@ObliviousHarmony
Copy link
Contributor Author

I took a look at implementing this as a top-level option only and it seems like there's going to need to be a refactor of the config parsing and validating to accommodate. Once that's done I'll revisit this!

@ObliviousHarmony
Copy link
Contributor Author

Now that the config refactor has landed I'm closing this in favor of #50196. This PR is not really actionable anymore, and given the change in scope (from environment-specific to root), it makes sense to just close it.

@ObliviousHarmony ObliviousHarmony deleted the try/env-post-install branch April 28, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Env /packages/env [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants