-
Notifications
You must be signed in to change notification settings - Fork 802
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
Adding a docker-based dev env #40934
base: trunk
Are you sure you want to change the base?
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
This comment was marked as outdated.
This comment was marked as outdated.
…unless specified to the dev container, like build-image
1be7f76
to
4391224
Compare
1541a10
to
4391224
Compare
…in the docker/data dir
…ther other containers
I wanted to test this from a fresh computer, so I tried to start from a computer running Ubuntu, where nothing was installed yet. Unfortunately, I run into an error when I run
I'm not sure what's causing this, and I'm not familiar enough with Ubuntu to debug this I'm afraid. |
…up, down, stop, clean
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.
Gave it a quick once-over.
NODE_VERSION: versionVars.NODE_VERSION, | ||
PNPM_VERSION: versionVars.PNPM_VERSION, | ||
COMPOSE_PROJECT_NAME: projectName, | ||
PORT_WORDPRESS: args.includes( '--type=e2e' ) ? '8889' : '80', |
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 will work for most people, so maybe it's fine, but I do note it will blow up if something else is already running on localhost port 80.
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.
nod, I believe this is the same as the existing jetpack docker
, except the existing command accepts an optional port arg
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.
More importantly, the existing command allows overriding the default via tools/docker/.env
, while your code here overrides tools/docker/.env
.
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.
Ahh, you're totally right. The .env
should now work after 5e8d778
tools/docker/docker-compose.yml
Outdated
@@ -49,3 +49,16 @@ services: | |||
environment: | |||
- HOST_PORT=${PORT_WORDPRESS:-80} | |||
- COMPOSE_PROJECT_NAME=$COMPOSE_PROJECT_NAME | |||
|
|||
monorepo: |
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 breaks normal use of jetpack docker up
.
$ jetpack docker up -d
WARNING: The HOST_CWD variable is not set. Defaulting to a blank string.
ERROR: build path /tools/docker either does not exist, is not accessible, or is not a valid URL.
Command exited with status 1
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 this should work now after e098e69
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.
$ jetpack docker up
ERROR: build path /usr/local/src/automattic/jetpack/tools/docker/tools/docker either does not exist, is not accessible, or is not a valid URL.
Command exited with status 1
Fixing that will, for me, likely still run into https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1087370. Although we probably don't need to consider me personally as a blocker.
But even for everyone else that it does work for, it'll mean that jetpack docker up
will unnecessarily build that image to start an instance of the container which will then immediately exit.
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.
nod, thanks. I'll dig into it more than the quick-looking fix.
Co-authored-by: Brad Jorsch <[email protected]>
…o try/dev-in-docker
In addition to testing on my two production Apple Silicon (M1, M2) devices, I tested it on a clear Intel Mac using |
Testing moving development commands to within docker to avoid local variances or require specific versions of things to be installed locally.
Proposed changes:
Other information:
Jetpack product discussion
None yet.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Testing the new CLI
wordpress
container can't reachwordpress.org
, haven't dug into that]. Restart Docker if applicable.npm i -g @automattic/jetpack-cli
cd /tmp
(or somewhere else without Jetpack)jp -v
(confirm it is installed, 1.0.0-beta1 is the current version)jp --help
-- it should report it can't find a monorepo checkout and invite you to usejp init
jp init
-- follow promptscd jetpack
(or whatever dir)git checkout try/dev-in-docker
to get on this branch.jp --help
-- this should pull the new dev container (I manually pushed it to docker hub for the sake of testing)jp build plugins/jetpack --with-deps
(does it work?)jp docker up -d
-- the CLI output will not be the expected "visit localhost". it'll say it's done before it's ready (need to fix).localhost
ensure it is the WP install.jp docker install
-- should run the usual install process, localhost should show a regular site.jp docker down
Testing the existing setup.
jetpack
commands work.jetpack --help
,jetpack build plugins/jetpack --with-deps
jetpack docker up -d
node_modules
dir. If you want to go back and forth between the two, you'll need to nuke thenode_modules
each time you switch.Does everything still work as expected? If not, does clearing the docker containers/images resolve it? Please note that.
The intent of the PR as this point is create a new, rough, beta way to dev within docker without impacting the existing
jetpack
commands running locally for devs who have things working. I hope to quickly iterate to the dev-in-docker is a reliable way for anyone to jump into, but the local way of doing it should still work too (and for the foreseeable future).Known issue: Git Hooks do not work on a fresh install using only
jp
for dev. It'll commit, push, etc without running the hooks.