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

Env: Migrate to Compose V2 #51339

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

lithrel
Copy link
Contributor

@lithrel lithrel commented Jun 8, 2023

Fixes #51249

What?

This PR updates docker-compose command to v2 docker compose.

Why?

As explained in #51249 , docker-compose v1 is now deprecated in favor of v2.

How?

Testing Instructions

Run all modified commands:

  • wp-env start
  • wp-env logs
  • wp-env run cli wp user list
  • wp-env stop
  • wp-env clean
  • wp-env destroy

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 8, 2023
@github-actions
Copy link

github-actions bot commented Jun 8, 2023

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @lithrel! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@lithrel lithrel force-pushed the update/docker-compose-v2 branch 3 times, most recently from d40eed0 to 8369535 Compare June 8, 2023 14:28
@skorasaurus skorasaurus added the [Tool] Env /packages/env label Jun 9, 2023
@noahtallen
Copy link
Member

noahtallen commented Jun 13, 2023

The unit test error is unfortunate:

SyntaxError: Cannot use import statement outside a module

I think this is because wp-env is still using CJS and not ESM, but this 3rd party dependency (yaml) might not provide a CJS option.

I don't know if there's a decent workaround -- we might have to explore migrating wp-env to ESM (which would be nice to do eventually anyways, but I don't know if there are blockers to that in the Gutenberg repo)

@swissspidy
Copy link
Member

/home/runner/work/gutenberg/gutenberg/packages/env/node_modules/yaml/browser/index.js:3
    import * as YAML from './dist/index.js'

I think this is because wp-env is still using CJS and not ESM, but this 3rd party dependency (yaml) might not provide a CJS option.

It's weird. It does have a CJS build under dist/index.js, see https://unpkg.com/browse/yaml@2.3.1/package.json. Shouldn't Node pick that export?

@noahtallen
Copy link
Member

Shouldn't Node pick that export?

Hm, I'd think so! 🤔 Maybe an issue since we're still using node 14? (#52363 )

@github-actions
Copy link

github-actions bot commented Aug 11, 2023

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: First-time Contributor, [Package] Env.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

@lithrel
Copy link
Contributor Author

lithrel commented Aug 11, 2023

Maybe an issue since we're still using node 14? (#52363 )

I just rebased to get Node 16, and same error apparently 🤔
I can get those tests to pass by adding in Jest config

"transformIgnorePatterns": [
    	"/node_modules/(?!(docker-compose|yaml)/)",
	"\\.pnp\\.[^\\/]+$"
],

but I feel like I'm missing something more obvious

@noahtallen
Copy link
Member

I'd recommend discarding all package-lock changes and then run npm install again! I wonder if we could actually write wp-env with ESM now because nothing imports its code

@noahtallen
Copy link
Member

I think transformIgnorePatterns is a fine workaround -- in this case, it does look like they need to be transpiled for Jest to use them. If those packages don't provide CJS output, and we don't use ESM, we won't be compatible. Normally, packages provide both types for compatibility, but I guess not in this case

@lithrel
Copy link
Contributor Author

lithrel commented Sep 22, 2023

@noahtallen I tested removing package-lock and reinstalling, but still the same issue. So I implementend the transformIgnorePatterns workaround (and also changed a line in documentation), it's passing now 👍

If you have a ticket for converting this package to ESM I can take a look in the following weeks 👀

@ObliviousHarmony
Copy link
Contributor

It looks like the resolution problem is because of the conditional exports in yaml being incompatible with Jest. As far as I can tell, the standard is for "import" and "require" to be the conditional exports but yaml uses "node" and "default" instead. This ends up being resolved incorrectly and it tries to load the browser code as default. I'm going to submit an upstream fix to "yaml " and then once that gets released an upstream fix to "docker-compose".

While we wait for that though, I think it would be worthwhile to get this PR across the finish line. docker-compose has been unsupported for quite some time and this change is likely fine as long as we mark it as major. Before we merge though, we should update docker-compose again since it has been a long time 😄

@lithrel
Copy link
Contributor Author

lithrel commented Nov 17, 2023

Before we merge though, we should update docker-compose again since it has been a long time 😄

@ObliviousHarmony Rebased and updated docker-compose to 0.24.3 👍

Copy link
Contributor

@ObliviousHarmony ObliviousHarmony left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the rebase @lithrel! This looks good, I'm happy with it. Could you add a changelog entry and then we can merge it?

@lithrel lithrel force-pushed the update/docker-compose-v2 branch 2 times, most recently from 6aa187a to b5526bc Compare November 22, 2023 06:27
@lithrel
Copy link
Contributor Author

lithrel commented Nov 22, 2023

Done @ObliviousHarmony :)

@t-hamano
Copy link
Contributor

Run all modified commands:

  • wp-env start
  • wp-env logs
  • wp-env run cli wp user list
  • wp-env stop
  • wp-env clean
  • wp-env destroy

I would like to inform you that all of these commands worked correctly on Windows 11 🎉

Copy link
Contributor

@ObliviousHarmony ObliviousHarmony left a comment

Choose a reason for hiding this comment

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

Thanks, everything worked for me locally just fine too 😄

@noahtallen
Copy link
Member

Looks like we just need to fix the changelog conflict, and then I'm happy to merge!

Update docker-compose package to 0.24.1
Use v2 as dockerCompose command

Fixes WordPress#51249
@lithrel
Copy link
Contributor Author

lithrel commented Dec 5, 2023

@noahtallen rebased/resolved 👍

@ObliviousHarmony ObliviousHarmony merged commit a0943f2 into WordPress:trunk Dec 5, 2023
50 checks passed
@github-actions github-actions bot added this to the Gutenberg 17.3 milestone Dec 5, 2023
@ockham
Copy link
Contributor

ockham commented Dec 16, 2023

👋 FWIW, npm run wp-env start has recently started failing for me; I believe it was correlated with this change. The error message is as follows:

> gutenberg@17.3.0-rc.1 wp-env
> wp-env start

✖ Error while running docker compose command.
unknown shorthand flag: 'f' in -f
See 'docker --help'.

Usage:  docker [OPTIONS] COMMAND

The reason seems to be that docker-compose is now a Docker plugin (rather than a separate script or executable) that wasn't installed on my system (and hence docker compose -f ... was failing).

I’m using colima on my system, and have installed it — and Docker — via brew. I found the solution to my issue here:

Compose is now a Docker plugin. For Docker to find this plugin, symlink it:
    mkdir -p ~/.docker/cli-plugins
    ln -sfn $HOMEBREW_PREFIX/opt/docker-compose/bin/docker-compose ~/.docker/cli-plugins/docker-compose

After running those commands, npm run wp-env start is working again 🎉

Just sharing in case anyone is experiencing the same issue.

@noahtallen
Copy link
Member

Very nice! I have Docker installed with Docker Desktop, and it seemed to work out of the box with that setup.

@swissspidy
Copy link
Member

FYI this did not properly update the version of docker-compose in the lock file, which caused some issues later on as a new version of docker-compose was released with a breaking change.

See #59183, #6133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Tool] Env /packages/env
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wp-env - Migrate to Compose V2
7 participants