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

wp-env: Add custom port numbers to .wp-env.json #20158

Merged
merged 11 commits into from Feb 13, 2020

Conversation

@noahtallen
Copy link
Contributor

noahtallen commented Feb 11, 2020

Description

This PR adds support for custom port numbers in the .wp-env.json file.

Here's why I like having this option:

  • It can make sense for a team to use the same base URL for development
  • In the case of e2e tests, when you specify the URL, the port numbers would always be consistent
  • It's sort of clunky to start every call to wp-env start with the port numbers if you need them to be different every time
  • There are some oddities in browsers/docker when you change the port number all the time. If you forget to include the different port number, the browser can start redirecting you to the default port number.

This does not change the behavior of the environment variables; they still take precedent over values in .wp-env.json. The default port numbers also stay the same.

How has this been tested?

I ran this code on my local plugin and verified that:

  • Invalid port numbers were not accepted
  • Having the same port number was discarded
  • The default values are the same as before if not specified
  • Custom values are added to the docker-compose file

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Feb 11, 2020

noahtallen added a commit to Automattic/wp-calypso that referenced this pull request Feb 11, 2020
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

noahtallen commented Feb 11, 2020

This should support arbitrary environment variables and you should also be
able to scope them to a specific container.

Neat idea!

Possible API:

{
  "plugins": [ "." ],
  "environment": {
    "mysql": {
      "MYSQL_ALLOW_EMPTY_PASSWORD": "no"
     },
    "wordpress": {
      "WP_ENV_PORT": "12345"
    }
  }
}

Then we could spread the environment variables into each of the docker services.

	return {
		version: '3.7',
		services: {
			mysql: {
				image: 'mariadb',
				environment: {
					MYSQL_ALLOW_EMPTY_PASSWORD: 'yes',
					...( config.environment.mysql || { } ),
				},
			},
		},
	};
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Feb 11, 2020

That looks sensible to me but don’t forget global ones.

@noahtallen

This comment has been minimized.

Copy link
Contributor Author

noahtallen commented Feb 11, 2020

Per global ones, do you mean shell? i.e. have a way to specify:

  1. Environment variables in the container (like LOCAL_DIR in var/www/$LOCAL_DIR)
  2. Environment variables for executing docker-compose (i.e. WP_ENV_PORT)

In that case, we could have two different options:

{
  "plugins": [ "." ],
  "shellEnv": {
    "WP_ENV_PORT": "12345"
  },
  "dockerEnv": {
    "mysql": {
      "MYSQL_ALLOW_EMPTY_PASSWORD": "no"
     },
    "wordpress": {
      "LOCAL_DIR": "12345"
    }
  }
}

The shellEnv would be written to the .env file, and dockerEnv would be added to the docker-config file (as above)

@noahtallen noahtallen mentioned this pull request Feb 11, 2020
1 of 2 tasks complete
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Feb 12, 2020

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Feb 13, 2020

Apologies in advance, I have opinions! 😅

Environment variables

Is being able to customise environment variables in this way useful? Beyond WP_ENV_PORT, what other environment variables are developers likely to want to customise?

I fear that this would detract from a key part of wp-env's design which is that it's zero-config and provides decisions, not options. wp-env does not need to do everything. Developers who need options can learn and use docker-compose.

Moreover, wp-env makes a lot of assumptions about how exactly its docker containers are configured. For example, if MYSQL_ALLOW_EMPTY_PASSWORD is changed, then wp-env will not work. Allowing too much customisation here may make it very difficult for us to help developers with issues they run into.

Customising the port

There are three places where configuration for wp-env can come from: command line options, environment variables, and .wp-env.json config. In my opinion:

  1. wp-env.json is best suited for configuration that is per project.
  2. Environment variables are best suited for configuration that is per terminal session.
  3. Command line options are best suited for configuration that is per command.

I'd argue that the port setting fits most neatly into (1).

This aligns with the design of docker-compose, too, which is the command line interface that I think wp-env most resembles. There, you specify the port in docker-compose.yaml and not through an environment variable or a --port option when running docker-compose up.

My ideal API for this would be that the port is specified in .wp-env.json and that any .wp-env.json field can be overridden using a command line option, for example:

wp-env start --port 3000 --core WordPress/WordPress#5.2
packages/env/lib/config.js Outdated Show resolved Hide resolved
packages/env/lib/config.js Outdated Show resolved Hide resolved
packages/env/lib/config.js Outdated Show resolved Hide resolved
packages/env/lib/build-docker-compose-config.js Outdated Show resolved Hide resolved
packages/env/lib/build-docker-compose-config.js Outdated Show resolved Hide resolved
packages/env/README.md Outdated Show resolved Hide resolved
noahtallen and others added 4 commits Feb 13, 2020
Co-Authored-By: Robert Anderson <robert@noisysocks.com>
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

noahtallen commented Feb 13, 2020

Thanks for the suggestions @noisysocks, everything should be fixed now. I also added some tests to cover the port number validation.

I do agree with @epiqueras that adding environment variable support would be very powerful, but I also agree with @noisysocks take not having it keeps the tool simpler.

At any rate, we could implement that in follow up if we really want it, and still have things like port numbers available through .wp-env.json and have it as a "per-project setting" that is officially supported.

So with that, I think this PR is ready to go. :)

Copy link
Member

noisysocks left a comment

Looking good!

Let's update packages/env/CHANGELOG.md to note this new functionality and then I think we're good to go.

packages/env/lib/config.js Outdated Show resolved Hide resolved
packages/env/lib/config.js Outdated Show resolved Hide resolved
noahtallen and others added 2 commits Feb 13, 2020
Co-Authored-By: Robert Anderson <robert@noisysocks.com>
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

noahtallen commented Feb 13, 2020

@noisysocks Added. Do I need to bump the package.json version as well?

packages/env/CHANGELOG.md Outdated Show resolved Hide resolved
packages/env/CHANGELOG.md Outdated Show resolved Hide resolved
@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Feb 13, 2020

@noisysocks Added. Do I need to bump the package.json version as well?

No, that's done when packages are next published.

@noahtallen noahtallen force-pushed the feature/wp-env-custom-port-numbers branch from 55f5650 to c9c2f21 Feb 13, 2020
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Feb 13, 2020

Thanks for the review, @noisysocks! I think it's wise to go with this and revisit more variable support later if/when the need arises.

@noahtallen noahtallen merged commit cf8fa0a into master Feb 13, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@noahtallen noahtallen deleted the feature/wp-env-custom-port-numbers branch Feb 13, 2020
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 13, 2020
noahtallen added a commit to Automattic/wp-calypso that referenced this pull request Feb 13, 2020
jorgefilipecosta added a commit that referenced this pull request Mar 2, 2020
Allows the user to set `port` and `testsPort` in .wp-env.json.

- Port must be a number
- Ports must be different
- Ports can still be overridden with the environment variables
jorgefilipecosta added a commit that referenced this pull request Mar 2, 2020
Allows the user to set `port` and `testsPort` in .wp-env.json.

- Port must be a number
- Ports must be different
- Ports can still be overridden with the environment variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.