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

Update docker compose #902

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

davidphay
Copy link
Contributor

Update docker compose to the latest version (2.16.0) & install it as docker plugin on linux server since compose v1 will be deprecated on June 2023.

reference: https://docs.docker.com/compose/install/linux/

@davidphay davidphay requested a review from a team as a code owner February 24, 2023 17:55
@puppet-community-rangefinder
Copy link

docker::compose is a class

Breaking changes to this file WILL impact these 3 modules (exact match):

docker::params is a class

that may have no external impact to Forge modules.

This module is declared in 6 of 580 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@kenyon
Copy link
Contributor

kenyon commented Feb 24, 2023

Looks like this intends to fix #891.

@fraenki
Copy link

fraenki commented Mar 6, 2023

@davidphay I've noticed a small issue while testing this PR:

$ file /usr/local/bin/docker-compose-2.16.0
/usr/local/bin/docker-compose-2.16.0: ASCII text, with no line terminators

$ cat /usr/local/bin/docker-compose-2.16.0
Not Found

It seems like the version 1.x download URL does not work for version 2.x:

$ curl -v https://github.com/docker/compose/releases/download/2.16.0/docker-compose-Linux-x86_64 
...
< HTTP/2 404 
< server: GitHub.com

On Docker Compose version 2.x the version number must be prefixed with a v in the download URL:

$ curl -v https://github.com/docker/compose/releases/download/v2.16.0/docker-compose-Linux-x86_64
...
< HTTP/2 302 
< server: GitHub.com

@davidphay
Copy link
Contributor Author

@davidphay I've noticed a small issue while testing this PR:

$ file /usr/local/bin/docker-compose-2.16.0
/usr/local/bin/docker-compose-2.16.0: ASCII text, with no line terminators

$ cat /usr/local/bin/docker-compose-2.16.0
Not Found

It seems like the version 1.x download URL does not work for version 2.x:

$ curl -v https://github.com/docker/compose/releases/download/2.16.0/docker-compose-Linux-x86_64 
...
< HTTP/2 404 
< server: GitHub.com

On Docker Compose version 2.x the version number must be prefixed with a v in the download URL:

$ curl -v https://github.com/docker/compose/releases/download/v2.16.0/docker-compose-Linux-x86_64
...
< HTTP/2 302 
< server: GitHub.com

Thanks, I fixed the issue !

@vegaaz
Copy link

vegaaz commented Mar 18, 2023

Wouldn’t it be a preferred way to install docker compose v2 via “docker-compose-plugin” package as described here https://docs.docker.com/compose/install/linux/#install-using-the-repository ?
Also setting the version via params leads to technical debt in my mind - do you agree ?

@kenyon
Copy link
Contributor

kenyon commented Mar 19, 2023

Wouldn’t it be a preferred way to install docker compose v2 via “docker-compose-plugin” package as described here https://docs.docker.com/compose/install/linux/#install-using-the-repository ? Also setting the version via params leads to technical debt in my mind - do you agree ?

I agree on both.

@vegaaz
Copy link

vegaaz commented Mar 19, 2023

Wouldn’t it be a preferred way to install docker compose v2 via “docker-compose-plugin” package as described here https://docs.docker.com/compose/install/linux/#install-using-the-repository ?
Also setting the version via params leads to technical debt in my mind - do you agree ?

I started working on this issue too, but I am currently busy…
Maybe this can help you ?

if $ensure == 'present' {
    if $facts['os']['family'] == 'windows' {
      # will be defined soon
      # https://docs.docker.com/compose/install/other/
    }
    else {
      case $facts['os']['family'] {
        'Debian': {
          ensure_packages('docker-compose-plugin', { ensure => pick($version,$ensure), require => defined('$docker::use_upstream_package_source') ? { true => Apt::Source['docker'], false => undef } }) #lint:ignore:140chars
        }
        'RedHat': {
          ensure_packages('docker-compose-plugin', { ensure => pick($version,$ensure), require => defined('$docker::use_upstream_package_source') ? { true => Yumrepo['docker'], false => undef } }) #lint:ignore:140chars lint:ignore:unquoted_string_in_selector
        }
        default: {}
      }
    }
  }

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2023

CLA assistant check
All committers have signed the CLA.

@davidphay
Copy link
Contributor Author

@kenyon Hello, I updated the code, is it good for you now ?

Copy link
Contributor

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Looks OK, we'll have to see what the tests say.

@davidphay
Copy link
Contributor Author

@kenyon linting issue should be fixed

@kenyon
Copy link
Contributor

kenyon commented Aug 25, 2023

We need someone from @puppetlabs to approve the GitHub Actions jobs.

Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

It looks like rubocop has some complaints. You can run it locally with bundle exec rubocop and probably auto-correct some offenses with bundle exec rubocop -a.

Also, can you please update the PR title / description (first message) to give more context about the usage of plugins: changelog entries link to the corresponding PR and since it is a breaking change, it's worth providing the most relevant info in the first message to the end user without the need to read all the implementation work in the discussion 😉

@@ -9,7 +9,7 @@

has_command(:docker, 'docker')

has_command(:dockercompose, 'docker-compose')
has_command(:dockercompose, 'docker')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since all usage of dockercompose is now replaced by docker, this line can be removed.

@fraenki
Copy link

fraenki commented Oct 16, 2023

@davidphay The reviewer added some notes, would you mind to fix these two things? Would love to finally see this PR getting merged. 😊

@ttousai
Copy link

ttousai commented Oct 19, 2023

@davidphay Thank you for the work you've done on this PR, it has saved me some work. I'd love to get it merged as well and I am available to assist if you don't mind.

@robertc99
Copy link

its become more critical that this gets merged since docker-compose v1 has become unreliable.

@robertc99
Copy link

Testing this code. Appears to work ok. Does generate the following warning
Warning: This function is deprecated, please use stdlib::ensure_packages instead. at ["/etc/puppetlabs/code/environments/stagingcloudv3/r10k/modules/docker/manifests/compose.pp", 26]

@kenyon
Copy link
Contributor

kenyon commented Jul 4, 2024

This should be closed now that #975 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support docker-compose-plugin as provider for docker_compose resource
9 participants