Switch to `docker compose` v2 #352

karkraeg · 2023-06-06T10:25:45Z

closes docker-compose reaches EOL in June #348

❤️ Thank you for your contribution!

Description

As described in #348 invenio-cli internally uses the deprecated version 1x of docker-compose to orchestrate containers. This forces users to install the old version alongside the newer version of docker compose. This pull request changes the underlying subcommands to use the new syntax.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

I'm aware of the code of conduct.
I've identified the copyright holder(s) and updated copyright headers for touched files (>15 lines contributions).
I've NOT included third-party code (copy/pasted source code or new dependencies).

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

You license your contribution under the same terms as the current repository’s license.
You agree that you have the right to license your contribution under the current repository’s license.

karkraeg · 2023-06-06T10:41:37Z

Hi @fenekku I can't associate it with the "PR Community" project

fenekku

Thanks for the contribution! The npm/node version changes is the only thing that should really be put aside for now.

fenekku · 2023-06-06T17:42:23Z

invenio_cli/commands/requirements.py

@@ -113,7 +114,7 @@ def check_docker_version(cls, major, minor=-1, patch=-1, exact=False):
        # Output comes in the form of
        # 'Docker version 19.03.13, build 4484c46d9d\n'
        try:
-            result = run_cmd(["docker", "--version"])
+            result = run_cmd(["docker", "version"])


For other reviewers/maintainers: what should that change mean for the version of invenio-cli ?

On the one hand, this is a breaking change for an installation that uses docker engine < 20.10

On the other, what is breaking is not invenio-cli public API. This is a change of a dependency of this project to keep up with docker support. It falls in the https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api category as a minor or patch change. (I don't have preferences for minor or patch)

Personally, I do lean in favor of a non-major bump given the above.

We list in the system requirements docs Docker 20.10.10+ since we migrated the Docker images.

As you said, there might be dangers with just upgrading Docker and running commands on existing containers/setups, but I think we never advertised docker-compose as a production setup so it's only going to be an issue during development.

I would say this is a minor bump, given how close we are to the June EOL of Docker v1.

@slint @fenekku Given the impact of this change, even if I agree that is not breaking change for the public API per see, I am leaning towards a major.... Even if it doesn't fall in this category https://semver.org/#what-if-i-inadvertently-alter-the-public-api-in-a-way-that-is-not-compliant-with-the-version-number-change-ie-the-code-incorrectly-introduces-a-major-breaking-change-in-a-patch-release I like the answer given there

Remember, Semantic Versioning is all about conveying meaning by how the version number changes. If these changes are important to your users, use the version number to inform them.

WDYT?

Yeah it's not super clear cut and semantic versioning creates or struggles with weird marketing expectations (From that link: "Semantic Versioning is all about conveying meaning by how the version number changes. If these changes are important to your users, use the version number to inform them." -- that's a lot of leeway 😓 ).

I lean for a minor bump per the "it's the dependency that changed and not the project's API" argument, as well as @slint's note on the pre-existing requirement change that didn't lead to a major bump. I think of docker-compose as a regualr python dependency/python version now and how there are typically no major version bump in a project as an older python version/package version is deprecated in favor of a new one.

fenekku · 2023-06-06T17:42:29Z

invenio_cli/commands/requirements.py

+            node_version = 18
+            npm_version = 9


Let's put those aside since they are not needed for the docker upgrade and InvenioRDM v11 still supports earlier version.

yes, sorry that was a change we did because we switched to node 18 with our custom theme. Will remove this - or can this change be kept out of the pull request somehow?

I don't know of a way to keep those out of the PR from our end. Yeah, make a new commit removing them and we can squash and merge when integrating the work.

did that in a04176d

fenekku · 2023-06-06T18:11:07Z

invenio_cli/helpers/docker_helper.py

@@ -34,7 +35,7 @@ def _normalize_name(self, project_shortname):
        Docker-Compose introduced support for dash and underscore in
        version 1.21.0.
        """
-        dc_version_string = run_cmd(["docker-compose", "--version"])
+        dc_version_string = run_cmd(["docker", "compose", "version"])


The search-and-replace changes work.

For extra credits perhaps: the repeated replacements also reveal how management of our interaction with docker isn't centralized. By factoring them together, we would only have 1 place to change and we could add compatibility/deprecation code easily if we wanted. Something like:

def __init__(self, project_shortname, local=True, log_config=None): """Constructor.""" super().__init__() self.container_prefix = self._normalize_name(project_shortname) self.local = local self.docker_client = docker.from_env() # NEW self.docker_compose = ["docker", "compose"] # or if we wanted warning/backwards support: if docker_version_less_than_20_10(): print("Warning you are using a deprecated version of Docker. Move to Docker 20.10+") self.deprecated_docker = True self.docker_compose = [...] else: self.docker_compose = [...] # ... def _normalize_name(self, project_shortname): # ... if self.deprecated_docker: dc_version_string = run_cmd(self.docker_compose + ["--version"]) else: dc_version_string = run_cmd(self.docker_compose + ["version"]) # ... # simpler for other other changes: def build_images(self, pull=False, cache=True) command = self.docker_compose + [ "--file", # ... ]

Similarly it's clear we should have been using DockerHelper in ServicesHealthCommands above.

Feel free to take or leave any of this (I may pick up whatever you leave for a future PR).

I'll look into this, will report later!

karkraeg · 2023-06-07T13:27:21Z

invenio_cli/commands/requirements.py

@@ -125,8 +126,9 @@ def check_docker_compose_version(cls, major, minor=-1, patch=-1, exact=False):
        """Check the docker compose version."""
        # Output comes in the form of
        # 'Docker Compose version v2.17.3\n'
+        docker_helper = DockerHelper("", local=False)


I guess this is not how it's supposed to be, but I can't call cli_config.get_project_shortname() as in

invenio-cli/invenio_cli/commands/services.py

Line 30 in 5f89470

cli_config.get_project_shortname(), local=True

in this context.

slint

LGTM, some minor points. If others agree, we can merge and schedule a minor release of invenio-cli.

slint · 2023-06-26T13:38:36Z

invenio_cli/commands/requirements.py

@@ -113,7 +114,7 @@ def check_docker_version(cls, major, minor=-1, patch=-1, exact=False):
        # Output comes in the form of
        # 'Docker version 19.03.13, build 4484c46d9d\n'
        try:
-            result = run_cmd(["docker", "--version"])
+            result = run_cmd(["docker", "version"])


We list in the system requirements docs Docker 20.10.10+ since we migrated the Docker images.

As you said, there might be dangers with just upgrading Docker and running commands on existing containers/setups, but I think we never advertised docker-compose as a production setup so it's only going to be an issue during development.

I would say this is a minor bump, given how close we are to the June EOL of Docker v1.

slint · 2023-06-26T13:40:37Z

invenio_cli/helpers/docker_helper.py

@@ -24,6 +25,7 @@ class DockerHelper(object):
    def __init__(self, project_shortname, local=True, log_config=None):
        """Constructor."""
        super().__init__()
+        self.docker_compose = ["docker", "compose"]


comment: at this point I'm even sceptical of if we need the class attribute self.docker_compose, or if we can just replace everywhere docker-compose -> docker compose.

Not blocking for merging this PR, but something to possibly shelve and explore later as a task to simplify the codebase.

Right, with the clarity that we don't need to fret about backwards support "docker", "compose" would have been a fine alternative too.

Later down the road, I do think centralizing all the docker/docker-compose calls to DockerHelper would be the way to go, so that we better isolate docker related code.

jennur

Looks good to us, but agree with Alex that we should be consistent with hardcoding ["docker", "compose"]or using the variable docker_compose.

ppanero

LGTM, I think the corresponding docs should also be updated when/before this is merged.

ppanero · 2023-06-27T11:38:40Z

invenio_cli/commands/requirements.py

@@ -113,7 +115,7 @@ def check_docker_version(cls, major, minor=-1, patch=-1, exact=False):
        # Output comes in the form of
        # 'Docker version 19.03.13, build 4484c46d9d\n'
        try:
-            result = run_cmd(["docker", "--version"])
+            result = run_cmd(["docker", "version"])


update comment in line 19

docker version outputs the version like this:

Client: Docker Engine - Community Version: 24.0.2 API version: 1.43 ...

as opposed to docker --version: Docker version 24.0.2, build cb74dfc

The classmethod _version_from_string works because it selects the first regex match, but I think it would actually be better to keep the --version query.

Or which comment are you referring to?

A more "reliable" alternative would be to use docker version --format json which returns JSON with the version string under the Client.Version key:

$ docker version -f json | jq { "Client": { "Platform": { "Name": "Docker Engine - Community" }, "Version": "24.0.2", "ApiVersion": "1.43", "DefaultAPIVersion": "1.43", "GitCommit": "cb74dfc", "GoVersion": "go1.20.4", "Os": "linux", "Arch": "amd64", "BuildTime": "Thu May 25 21:55:21 2023", "Context": "default" }, "Server": { "Platform": { "Name": "Docker Engine - Community" }, "Components": [ { "Name": "Engine", "Version": "24.0.2", "Details": { "ApiVersion": "1.43", "Arch": "amd64", "BuildTime": "Thu May 25 21:54:24 2023", "Experimental": "false", "GitCommit": "659604f", "GoVersion": "go1.20.4", "KernelVersion": "3.10.0-1127.19.1.el7.x86_64", "MinAPIVersion": "1.12", "Os": "linux" } }, { "Name": "containerd", "Version": "1.6.9", "Details": { "GitCommit": "1c90a442489720eec95342e1789ee8a5e1b9536f" } }, { "Name": "runc", "Version": "1.1.4", "Details": { "GitCommit": "v1.1.4-0-g5fd4c4d" } }, { "Name": "docker-init", "Version": "0.19.0", "Details": { "GitCommit": "de40ad0" } } ], "Version": "24.0.2", "ApiVersion": "1.43", "MinAPIVersion": "1.12", "GitCommit": "659604f", "GoVersion": "go1.20.4", "Os": "linux", "Arch": "amd64", "KernelVersion": "3.10.0-1127.19.1.el7.x86_64", "BuildTime": "2023-05-25T21:54:24.000000000+00:00" } }

This would mean that we would have to chamnge check_docker_version to something like this, should I add this to this PR?

@classmethod def check_docker_version(cls, major, minor=-1, patch=-1, exact=False): """Check the docker version.""" # Use JOSN Formatted output and parse it try: result = run_cmd(["docker", "version", "--format", "json"]) result_json = json.loads(result.output.strip()) version = cls._version_from_string(result_json["Client"]["Version"]) return cls._check_version("Docker", version, major, minor, patch, exact) except Exception as err: return ProcessResponse(error=f"Docker not found. Got {err}.", status_code=1)

LGTM and I think it brings us a bit closer to having a "stable" way of parsing the output, and if the option is there by Docker, I imagine they plan to keep it around.

invenio_cli/commands/requirements.py

slint

LGTM, one comment only to shelve and address later.

karkraeg · 2023-07-24T09:58:53Z

Hi, I updated my fork with your latest changes. Is there anything blocking the merging that I could help with? Tbh I somehow lost track.

fenekku · 2023-07-24T12:35:39Z

Everyone approved, tests pass and it's been out very long: I am merging. Thank you @karkraeg for all the effort, we appreciate it!

karkraeg added 2 commits June 6, 2023 12:02

use docker compose v2

eb7b0ff

fixed formatting with black

8b210e0

fenekku self-requested a review June 6, 2023 16:55

fenekku approved these changes Jun 6, 2023

View reviewed changes

karkraeg added 3 commits June 7, 2023 11:16

added docker-compose attribute to DockerHelper class

a1a934e

revert npm and node version bump in requirements check

a04176d

used isort

7afee22

karkraeg commented Jun 7, 2023

View reviewed changes

utnapischtim requested a review from ppanero June 16, 2023 07:42

slint approved these changes Jun 26, 2023

View reviewed changes

jennur approved these changes Jun 26, 2023

View reviewed changes

ppanero approved these changes Jun 27, 2023

View reviewed changes

zzacharo approved these changes Jul 3, 2023

View reviewed changes

zzacharo reviewed Jul 3, 2023

View reviewed changes

invenio_cli/commands/requirements.py Show resolved Hide resolved

check docker version using JSON output

dfd0425

slint approved these changes Jul 18, 2023

View reviewed changes

slint mentioned this pull request Jul 18, 2023

docker: remove docker_helper.docker_compose usage #357

Open

Merge branch 'inveniosoftware:master' into master

4993310

fenekku merged commit fcb7d03 into inveniosoftware:master Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to `docker compose` v2 #352

Switch to `docker compose` v2 #352

karkraeg commented Jun 6, 2023 •

edited by kpsherva

Loading

karkraeg commented Jun 6, 2023

fenekku left a comment •

edited

Loading

fenekku Jun 6, 2023

slint Jun 26, 2023

zzacharo Jun 30, 2023

fenekku Jun 30, 2023

fenekku Jun 6, 2023

karkraeg Jun 7, 2023

fenekku Jun 7, 2023

karkraeg Jun 7, 2023

fenekku Jun 6, 2023

karkraeg Jun 7, 2023

karkraeg Jun 7, 2023

slint left a comment

slint Jun 26, 2023

slint Jun 26, 2023

fenekku Jun 26, 2023

jennur left a comment

ppanero left a comment

ppanero Jun 27, 2023

karkraeg Jun 28, 2023 •

edited

Loading

slint Jun 28, 2023 •

edited

Loading

karkraeg Jun 30, 2023

slint Jul 3, 2023

slint left a comment

karkraeg commented Jul 24, 2023

fenekku commented Jul 24, 2023

Switch to docker compose v2 #352

Switch to docker compose v2 #352

Conversation

karkraeg commented Jun 6, 2023 • edited by kpsherva Loading

Description

Checklist

karkraeg commented Jun 6, 2023

fenekku left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennur left a comment

Choose a reason for hiding this comment

ppanero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karkraeg Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

slint Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slint left a comment

Choose a reason for hiding this comment

karkraeg commented Jul 24, 2023

fenekku commented Jul 24, 2023

Switch to `docker compose` v2 #352

Switch to `docker compose` v2 #352

karkraeg commented Jun 6, 2023 •

edited by kpsherva

Loading

fenekku left a comment •

edited

Loading

karkraeg Jun 28, 2023 •

edited

Loading

slint Jun 28, 2023 •

edited

Loading