Use the docker-compose-plugin via 'docker compose' instead of 'docker-compose' #975

nathanlcarlson · 2024-04-11T16:40:05Z

Summary

Use the docker-compose-plugin via 'docker compose' instead of 'docker-compose'

From https://docs.docker.com/compose/install/

Docker's documentation refers to and describes Compose V2 functionality. Effective July 2023, Compose V1 stopped receiving updates and is no longer in new Docker Desktop releases. Compose V2 has replaced it and is now integrated into all current Docker Desktop versions. For more information, see Migrate to Compose V2.

Additional Context

Based on #902. Created my own PR to hopefully get this resolved sooner.

Related Issues (if any)

#891

Checklist

🟢 Spec tests.
🟢 Acceptance tests.
Manually verified. (For example puppet apply)

CLAassistant · 2024-04-11T16:40:11Z

All committers have signed the CLA.

nathanlcarlson · 2024-04-11T16:47:01Z

This link is dead https://puppetlabs.github.io/iac/docs/contributing_to_a_module.html

nathanlcarlson · 2024-04-11T16:57:08Z

pdk update will apply some changes. I'm not sure if the tests want those changes or not. I've opted to make this up-to-date with main rather than apply those changes. I've used pdk test unit as the test command. I'm not sure what that will run exactly.

nathanlcarlson · 2024-04-11T17:52:47Z

I've done some basic manual testing of this now, both with a fresh start of a compose project and with a running compose project.

smortex

It's better to focus on a single point for each PR (to minimize the changes to review) with commits that change a single aspect at a time (to make review easier).

For example 91ab09c does multiple things:

It updates PDK-related files (IMHO this should not be part of this PR unless if it fix CI failure)
It update lib/puppet/provider/docker_compose/ruby.rb to remove unused code (which should be part of this PR).

Do you think you can rework this PR and adjust the commits you add on top of #975 to only finish the work that was started by @davidphay, ignoring the PDK stuff, and without the merge commit? Maybe it is enough to have the CI green 😉

Thanks!

.rubocop.yml

nathanlcarlson · 2024-04-11T19:07:29Z

Sure I can do those things. Please fix your CONTRIBUTING.md.

nathanlcarlson · 2024-04-11T19:42:44Z

I've made a proper mess by reverting commits that I'd merged. I'll figure out a better way.

kenyon

A rebase on main will get rid of the merge commit.

There are changes in early commits that are then removed in later commits. It would be good to clean up the commit history.

manifests/compose.pp

nathanlcarlson · 2024-04-16T20:05:06Z

Aren't changes to previous commits the nature of applying requested changes? Besides remaking the branch and PR altogether I'd have to really stretch my git abilities to make it appear such that it was all done as desired from the beginning.

nathanlcarlson · 2024-04-16T20:26:33Z

I suppose maybe you want me to squash the commits together selectively? I can try that.

nathanlcarlson · 2024-04-16T20:47:35Z

Looking into squashing it, I'm not sure that will comply with "commits that change a single aspect at a time". I won't do anything without further advisement.

kenyon · 2024-04-16T21:09:32Z

Use interactive rebase to edit the commit history. I'm not saying squash all of the changes into one commit, unless that is what makes the most sense.

nathanlcarlson · 2024-04-16T22:08:06Z

Unfortunately it really only made sense to me to make it into a single commit.

kenyon

The be_include to include changes should be a separate commit, since they are unrelated to the other changes.

A couple of comments, otherwise looks good.

manifests/compose.pp

nathanlcarlson · 2024-04-17T15:04:37Z

I removed the be_include changes altogether as neither my pdk validate and pdk test unit complained about it after reverting.

kenyon · 2024-04-17T17:32:07Z

Looks OK, just need to get someone from Perforce to approve the workflow run to see test results.

rwaffen · 2024-04-18T15:35:40Z

Nice work! I'm curious about this change too!

nathanlcarlson · 2024-04-19T14:39:11Z

I've realized that my problem is not with the standalone docker-compose, but rather with the version that is installed by default. This brings me to consider this PR a bit unnecessary after all. It would be nice to have the compose plugin installed as a package as it is then managed and updated as any other package would be on Linux. This seems to come at the expense of Windows Server support, which seems like a poor trade off. Supporting both the package-based and the standalone compose strategies seems like a lot as well.

Perhaps upgrading the default installed version of the standalone docker-compose may be a preferable path forward? Either way I think I am leaning towards closing this PR unless someone with Windows Server knowledge sees a different path forward for it.

kenyon · 2024-04-21T04:53:55Z

@nathanlcarlson standalone docker-compose has been deprecated for a while (if I'm understanding you correctly), so I don't think there is any upgrading to do for that.

smortex · 2024-04-21T07:26:33Z

Perhaps upgrading the default installed version of the standalone docker-compose may be a preferable path forward?

IMHO default versions are not great because bumping them is a backwards-incompatible change, so it basically never happens. When they are installed as packages, the default value of present is reasonable because it allows to follow package updates and still allow users to pin a specific version if they have to. I like it a lot.

It would be nice to have the compose plugin installed as a package as it is then managed and updated as any other package would be on Linux.

💯

I understand there are 2 issues here:

the installation method is not great; and
the default version is not great.

Improving the installation method being backwards-incompatible, changing the version it install by default looks fine to me.

Upgrading to the next major version of the modules for users of docker-compose would require to:

Manually remove the docker-compose program that the older version of the module installed (or maybe we can automate this removal);
Explicitly indicate which version of the docker-compose-plugin to install if needed.

This feels reasonable to me.

nathanlcarlson · 2024-04-22T20:28:31Z

@kenyon Ahh, yes, I see that the Docker site reports it as "not recommended and is only supported for backward compatibility purposes".

I've applied some changes that attempt to repair and cleanup the acceptance tests.

manifests/compose.pp

rwaffen · 2024-05-31T08:49:30Z

is there something that could be done to help to move this forward?

nathanlcarlson · 2024-05-31T14:44:11Z

is there something that could be done to help to move this forward?

I was waiting for @smortex to reply on my response to their comments. I can clean up duplicate code, but I made a couple points about the other comments.

smortex · 2024-06-01T04:10:42Z

I was waiting for @smortex to reply on my response to their comments. I can clean up duplicate code, but I made a couple points about the other comments.

Dang! Too many notifications :-) I don't "own" the code, so feel free to add changes which allow other contributors to provide feedback too, and do not wait for me as my availability is somewhat sparse currently :-D I will reply at the right location to your points.

ghost · 2024-06-13T13:47:42Z

Any news here? Any help needed? Would be great to get this finally resolved.

These changes include updates to the docker compose related code and tests. Install compose as plugin on linux Simplify compose install clean class compose_spec.rb Fix linting issue Since all usage of dockercompose is now replaced by docker, this line can be removed Fix syntax and indentation Use the package resource directly

…eclarations

nathanlcarlson · 2024-06-13T15:05:33Z

I've pushed some commits that attempt to make the requested changes.

Ramesh7

The changes looks good to me, but will wait for @smortex @kenyon feedback here.

kenyon

Did some tests with this code on a node where I use docker_compose, worked for me!

nathanlcarlson requested a review from a team as a code owner April 11, 2024 16:40

ia-content added the community label Apr 11, 2024

smortex requested changes Apr 11, 2024

View reviewed changes

.rubocop.yml Outdated Show resolved Hide resolved

nathanlcarlson force-pushed the update_compose branch from 32dd642 to 6277484 Compare April 11, 2024 20:01

nathanlcarlson requested a review from smortex April 11, 2024 21:15

kenyon suggested changes Apr 16, 2024

View reviewed changes

manifests/compose.pp Outdated Show resolved Hide resolved

nathanlcarlson force-pushed the update_compose branch from b6ababd to 21d48a1 Compare April 16, 2024 19:36

nathanlcarlson force-pushed the update_compose branch from e82d6bd to 97efd64 Compare April 16, 2024 22:05

kenyon reviewed Apr 16, 2024

View reviewed changes

manifests/compose.pp Outdated Show resolved Hide resolved

manifests/compose.pp Outdated Show resolved Hide resolved

nathanlcarlson force-pushed the update_compose branch from 97efd64 to ece0e8d Compare April 17, 2024 14:59

smortex added the backwards-incompatible label Apr 18, 2024

smortex reviewed Apr 22, 2024

View reviewed changes

manifests/compose.pp Show resolved Hide resolved

manifests/compose.pp Outdated Show resolved Hide resolved

manifests/compose.pp Outdated Show resolved Hide resolved

manifests/compose.pp Outdated Show resolved Hide resolved

manifests/compose.pp Show resolved Hide resolved

davidphay and others added 13 commits June 13, 2024 09:59

Use simplified equivalent condition to select require

48568ff

Resolve puppet-lint by not intermingling conditionals with resource d…

446337c

…eclarations

Update specific docker-compose-plugin version for testing

daa08de

Improve docker compose fail message for Windows

3938ea8

Repair acceptance tests for the docker-compose-plugin package

54c185a

Remove unused docker-compose files and parameters

88812a8

Resolve rubocop offense

d1cc5cb

Ensure parent scope not limited to docker::params

4add8a3

Ensure docker::params are available

5f557cf

Revise fail messages

1a540d1

Ensure the apt update occurs prior to package install

85f6ccd

De-duplicate code for package installation

053c64e

nathanlcarlson force-pushed the update_compose branch from 373a33f to 053c64e Compare June 13, 2024 15:00

Include docker to use ::manage_package

e7bce6a

smortex self-requested a review June 20, 2024 00:18

Ramesh7 approved these changes Jul 3, 2024

View reviewed changes

kenyon approved these changes Jul 3, 2024

View reviewed changes

Ramesh7 merged commit 6a566e1 into puppetlabs:main Jul 4, 2024
21 of 26 checks passed

kenyon mentioned this pull request Jul 4, 2024

Update docker compose #902

Open

jnk0 mentioned this pull request Jul 5, 2024

Switch to Compose V2 knoppi/puppet-jitsi#5

Merged

kenyon mentioned this pull request Aug 22, 2024

Support docker-compose-plugin as provider for docker_compose resource #891

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the docker-compose-plugin via 'docker compose' instead of 'docker-compose' #975

Use the docker-compose-plugin via 'docker compose' instead of 'docker-compose' #975

nathanlcarlson commented Apr 11, 2024 •

edited

Loading

CLAassistant commented Apr 11, 2024 •

edited

Loading

nathanlcarlson commented Apr 11, 2024

nathanlcarlson commented Apr 11, 2024

nathanlcarlson commented Apr 11, 2024

smortex left a comment

nathanlcarlson commented Apr 11, 2024

nathanlcarlson commented Apr 11, 2024

kenyon left a comment

nathanlcarlson commented Apr 16, 2024

nathanlcarlson commented Apr 16, 2024

nathanlcarlson commented Apr 16, 2024

kenyon commented Apr 16, 2024

nathanlcarlson commented Apr 16, 2024 •

edited

Loading

kenyon left a comment

nathanlcarlson commented Apr 17, 2024

kenyon commented Apr 17, 2024

rwaffen commented Apr 18, 2024

nathanlcarlson commented Apr 19, 2024

kenyon commented Apr 21, 2024

smortex commented Apr 21, 2024

nathanlcarlson commented Apr 22, 2024

rwaffen commented May 31, 2024

nathanlcarlson commented May 31, 2024

smortex commented Jun 1, 2024

ghost commented Jun 13, 2024

nathanlcarlson commented Jun 13, 2024

Ramesh7 left a comment

kenyon left a comment

Use the docker-compose-plugin via 'docker compose' instead of 'docker-compose' #975

Use the docker-compose-plugin via 'docker compose' instead of 'docker-compose' #975

Conversation

nathanlcarlson commented Apr 11, 2024 • edited Loading

Summary

Additional Context

Related Issues (if any)

Checklist

CLAassistant commented Apr 11, 2024 • edited Loading

nathanlcarlson commented Apr 11, 2024

nathanlcarlson commented Apr 11, 2024

nathanlcarlson commented Apr 11, 2024

smortex left a comment

Choose a reason for hiding this comment

nathanlcarlson commented Apr 11, 2024

nathanlcarlson commented Apr 11, 2024

kenyon left a comment

Choose a reason for hiding this comment

nathanlcarlson commented Apr 16, 2024

nathanlcarlson commented Apr 16, 2024

nathanlcarlson commented Apr 16, 2024

kenyon commented Apr 16, 2024

nathanlcarlson commented Apr 16, 2024 • edited Loading

kenyon left a comment

Choose a reason for hiding this comment

nathanlcarlson commented Apr 17, 2024

kenyon commented Apr 17, 2024

rwaffen commented Apr 18, 2024

nathanlcarlson commented Apr 19, 2024

kenyon commented Apr 21, 2024

smortex commented Apr 21, 2024

nathanlcarlson commented Apr 22, 2024

rwaffen commented May 31, 2024

nathanlcarlson commented May 31, 2024

smortex commented Jun 1, 2024

ghost commented Jun 13, 2024

nathanlcarlson commented Jun 13, 2024

Ramesh7 left a comment

Choose a reason for hiding this comment

kenyon left a comment

Choose a reason for hiding this comment

nathanlcarlson commented Apr 11, 2024 •

edited

Loading

CLAassistant commented Apr 11, 2024 •

edited

Loading

nathanlcarlson commented Apr 16, 2024 •

edited

Loading