use docker compose v2 #163

fmcwalters-edited · 2024-02-15T11:39:14Z

docker compose v2 is part of Docker nowadays. Rather than calling a separate Python lib that calls docker-compose, call docker compose and use v2.
Fix the - and _ difference between v1 and v2.
This will need THOROUGH testing

PFM-827

Purpose of PR

Upgrade to use Docker Compose V2

Parts of the app this will impact

docker compose

Todos

Test on Linux machines, different CI builds with different packages

Additional information

This will require Docker with Compose v2

Related links

docker compose v2 is part of Docker nowadays. Rather than calling a separate Python lib that calls `docker-compose`, call `docker compose` and use v2. Fix the `-` and `_` difference between v1 and v2. This will need THOROUGH testing PFM-827

gchazot

I doubt this is going to work as-is. It would be very lucky that all of the features of docker-compose that kubetools uses are fully backwards-compatible in docker compose.

CHANGELOG.md

kubetools/dev/backends/docker_compose/docker_util.py

fmcwalters-edited · 2024-02-15T13:09:48Z

I doubt this is going to work as-is. It would be very lucky that all of the features of docker-compose that kubetools uses are fully backwards-compatible in docker compose.

This is why it's only in draft, it still needs a thorough test across all dev machine OSes, architectures and with as many packages/services as possible.

fmcwalters-edited · 2024-02-15T14:43:25Z

I'm also seeing

WARN[0000] network default: network.external.name is deprecated. Please set network.name with external: true

from running ktd destroy - whilst this does not break anything yet, I feel like at this stage it is worth fixing.

the old type, `network.external.name` is deprecated on v2. It still works but it may be removed in future. Change to new format. See: docker-archive/compose-cli#1856

this code will only ever be used in this version of the tool, and the `__version__` only checks the version of the _tool_ anyways. Also remove redundant print statements

if we create a container using compose v1 and we want to use or remove it using compose v2, we will need to convert its name to use the new format

there are times where the container name consists of multiple names or a name and then a number; for example, composename-container-name-1-1. We want to keep only `container-name-1`. Ensure this is possible

fmcwalters-edited · 2024-02-16T10:17:15Z

force push because the previous code was weird - it was just to deal with the cases where there was more than one middle name

fmcwalters-edited · 2024-02-16T11:02:49Z

we can test this locally using something like echo -n "cython <3" > /tmp/constraint.txt && PIP_CONSTRAINT=/tmp/constraint.txt pip install -e ~/apps/kubetools to build and install from this branch. Then ensure you're using v14.0.0 when you run ktd.

Use the most logical version number for the next major version of this. This version of Kubetools introduces some unavoidable breaking changes that we can work around. Document these in the README.

…mpose v2

add incompatible version of Docker Desktop to CHANGELOG and add more detail to README as to why we need to be careful with this update

fmcwalters-edited · 2024-02-27T11:35:51Z

I have put this up for a dev release - it has been tested to work (with caveats - see README) on Mac Intel i5 and Mac M3.
However - we need to test this on Linux systems and in particular on build servers.

README.md

kubetools/dev/backends/docker_compose/docker_util.py

suggest migrating to secure Docker registries, reword the suggestion to use the legacy builder Co-authored-by: Germain Chazot <[email protected]>

make the code easier to test by moving the name conversion into its own function and add test coverage also add test coverage for `dockerise_label`

fmcwalters-edited · 2024-02-28T13:20:45Z

force push only to clean up some mistakes I made with linting and syntax 🤦‍♂️

add `TROUBLESHOOTING` section to README to document the issue with `insecure-registries` and link this to ticket which is open and relevant, for `buildx`. remove anything from the CHANGELOG which belongs here

setup.py

gchazot

Just realised I had all those comments pending from months ago.

gchazot · 2024-03-11T10:09:53Z

kubetools/dev/backends/docker_compose/config.py

-                'external': {
-                    'name': 'dev',
-                },
+                'name': 'dev',


I'm wondering if we should take this opportunity to rename this "default" network used by ktd.

default here in the config is from the point of view of the project/container. This declares the default network for the project to be "external" (i.e. pre-existing outside this compose project) and named dev.

I think we should replace dev by something that clearly identifies it as used by ktd. Maybe ktd_dev could be an option.

Also, from a few tests I've run, it seems like other "environments" as created with --env option (or test in the case of ktd test), end up with a "temporary" network name (i.e. it's not "external" and IS deleted by the destroy operation) which looks like <appname>_<env>_default. I think it would be cool if that network name ended up being similarly, something like ktd_<env>.
This aspect probably requires a lot of work to implement, However it could be the base for a feature allowing to create "multi-app" environments, in a similar way as we can today do with the "default" environment.

gchazot · 2024-03-11T10:12:48Z

README.md

+If you try and run `ktd up` with reference to an unsecure registry, e.g. `http://docker-registry.example.net` and are affected by this bug, you will get an error message that is
+similar to the following:


Suggested change

If you try and run `ktd up` with reference to an unsecure registry, e.g. `http://docker-registry.example.net` and are affected by this bug, you will get an error message that is

similar to the following:

If you try and run `ktd up` with reference to an unsecure registry, e.g. `http://docker-registry.example.net` and are affected by this bug, you will get an error message that is similar to the following:

gchazot · 2024-03-11T12:56:21Z

tests/test_docker_util.py

+
+
+class TestDockerComposeNameConversion(TestCase):
+    # we need to check this works for dashes and underscores


Suggested change

# we need to check this works for dashes and underscores

gchazot · 2024-03-11T13:01:49Z

tests/test_docker_util.py

+from kubetools.dev.backends.docker_compose.docker_util import _get_container_name_from_full_name
+
+
+def generate_long_names(number_of_separators, separator):


See comment below, this function probably will disappear. But if it had to stay, then it needs to be renamed because it's confusing::

It is generating ONE name, not (several) _names

Why do they have to be "long" ? Is it important for the tests? What would not be tested if the names were "short" ? Note that this aspect of making tests more understandable does not belong in this function's name only, but in how/why the result of it is used in tests.

gchazot · 2024-03-11T13:05:11Z

tests/test_docker_util.py

+        for i in range(5):
+            long_name = generate_long_names(i, '-')
+            dockerised_name = dockerise_label(long_name)
+            self.assertEqual(dockerised_name, ascii_lowercase[:i+1])


I'm not quite sure but I have a feeling that we're using ascii_lowercase here as a "long string", which "could look like an app name". If that the case, then in my opinion it's a bad idea. It's a case of "wrong abstraction". Instead here we want to use names that could really be what they stand for, so that it's obvious to the reader what we're talking about. Put them in a list and iterate over them. If you're worried about repeating that list too much, extract it in a constant and reuse it in each place.

gchazot · 2024-03-11T14:16:34Z

kubetools/dev/backends/docker_compose/docker_util.py

+    # if the container was made in v1, it will be composename_container_N
+    converted_name = full_name.replace('_', '-')


Just to check, are we trying to create logic that is backwards compatible with compose V2?

I think that, in line with the change to README and with the major version change, we don´t need to support any backwards compatible logic here.

gchazot · 2024-03-11T14:16:42Z

tests/test_docker_util.py

+            dockerised_name = dockerise_label(long_name)
+            self.assertEqual(dockerised_name, ascii_lowercase[:i+1])
+
+    def test_container_name_from_full_name_dashes(self):


These test names don't explain the scenario that we're testing here.
What are the cases where this logic might be relevant?

fmcwalters-edited added 2 commits February 15, 2024 11:36

use docker compose v2

a361a02

docker compose v2 is part of Docker nowadays. Rather than calling a separate Python lib that calls `docker-compose`, call `docker compose` and use v2. Fix the `-` and `_` difference between v1 and v2. This will need THOROUGH testing PFM-827

linting

7cda8d3

gchazot reviewed Feb 15, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kubetools/dev/backends/docker_compose/docker_util.py Outdated Show resolved Hide resolved

fmcwalters-edited added 4 commits February 15, 2024 14:52

use new form of network config

f6c595e

the old type, `network.external.name` is deprecated on v2. It still works but it may be removed in future. Change to new format. See: docker-archive/compose-cli#1856

remove version check and print statements

a439c4c

this code will only ever be used in this version of the tool, and the `__version__` only checks the version of the _tool_ anyways. Also remove redundant print statements

convert v1 container names to v2

03fee73

if we create a container using compose v1 and we want to use or remove it using compose v2, we will need to convert its name to use the new format

deal with cases where container name has more than one middle name

b2958d3

there are times where the container name consists of multiple names or a name and then a number; for example, composename-container-name-1-1. We want to keep only `container-name-1`. Ensure this is possible

fmcwalters-edited force-pushed the PFM-827-docker-compose-v2-upgrade branch from 1591eec to b2958d3 Compare February 16, 2024 10:16

fmcwalters-edited added 8 commits February 19, 2024 17:19

use v14.0.0 for Kubetools with Docker Compose v2, update README

70ed8ef

Use the most logical version number for the next major version of this. This version of Kubetools introduces some unavoidable breaking changes that we can work around. Document these in the README.

readme formatting

696efb3

add unit tests for a complex named app to check compatibility with co…

fc59bbf

…mpose v2

add missing infra for test

3a4c3fe

add correct complex-webserver

add0755

use complex-named-app for cron

f594288

improve documentation

eb53740

add incompatible version of Docker Desktop to CHANGELOG and add more detail to README as to why we need to be careful with this update

ensure dev release adheres to semver format

9ff20c1

fmcwalters-edited marked this pull request as ready for review February 27, 2024 11:34

add reason in CHANGELOG why this is a dev release

8e7b32c

gchazot reviewed Feb 27, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

kubetools/dev/backends/docker_compose/docker_util.py Outdated Show resolved Hide resolved

fmcwalters-edited and others added 2 commits February 28, 2024 10:51

make workarounds more descriptive

4da3de5

suggest migrating to secure Docker registries, reword the suggestion to use the legacy builder Co-authored-by: Germain Chazot <[email protected]>

factor out code to convert container names, add unit tests

b9cc5a4

make the code easier to test by moving the name conversion into its own function and add test coverage also add test coverage for `dockerise_label`

fmcwalters-edited force-pushed the PFM-827-docker-compose-v2-upgrade branch from d7f3207 to b9cc5a4 Compare February 28, 2024 13:19

improve documentation for buildx issue, make CHANGELOG more concise

7a8b920

add `TROUBLESHOOTING` section to README to document the issue with `insecure-registries` and link this to ticket which is open and relevant, for `buildx`. remove anything from the CHANGELOG which belongs here

gchazot mentioned this pull request Mar 11, 2024

Use python docker compose #164

Merged

fmcwalters-edited added 2 commits April 2, 2024 15:51

Merge branch 'master' into PFM-827-docker-compose-v2-upgrade

bddb7ea

remove unused import

700658a

DilaraOflaz reviewed Jul 11, 2024

View reviewed changes

setup.py Show resolved Hide resolved

fmcwalters-edited and others added 2 commits July 11, 2024 15:09

Merge branch 'master' into PFM-827-docker-compose-v2-upgrade

2a39579

use full version number

0f3b1bf

gchazot reviewed Jul 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use docker compose v2 #163

use docker compose v2 #163

fmcwalters-edited commented Feb 15, 2024 •

edited

Loading

gchazot left a comment

fmcwalters-edited commented Feb 15, 2024

fmcwalters-edited commented Feb 15, 2024

fmcwalters-edited commented Feb 16, 2024

fmcwalters-edited commented Feb 16, 2024 •

edited

Loading

fmcwalters-edited commented Feb 27, 2024

fmcwalters-edited commented Feb 28, 2024

gchazot left a comment

gchazot Mar 11, 2024

gchazot Mar 11, 2024

gchazot Mar 11, 2024

gchazot Mar 11, 2024

gchazot Mar 11, 2024

gchazot Mar 11, 2024

gchazot Mar 11, 2024

		If you try and run `ktd up` with reference to an unsecure registry, e.g. `http://docker-registry.example.net` and are affected by this bug, you will get an error message that is
		similar to the following:



		class TestDockerComposeNameConversion(TestCase):
		# we need to check this works for dashes and underscores

		from kubetools.dev.backends.docker_compose.docker_util import _get_container_name_from_full_name


		def generate_long_names(number_of_separators, separator):

		# if the container was made in v1, it will be composename_container_N
		converted_name = full_name.replace('_', '-')

use docker compose v2 #163

Are you sure you want to change the base?

use docker compose v2 #163

Conversation

fmcwalters-edited commented Feb 15, 2024 • edited Loading

Purpose of PR

Parts of the app this will impact

Todos

Additional information

Related links

gchazot left a comment

Choose a reason for hiding this comment

fmcwalters-edited commented Feb 15, 2024

fmcwalters-edited commented Feb 15, 2024

fmcwalters-edited commented Feb 16, 2024

fmcwalters-edited commented Feb 16, 2024 • edited Loading

fmcwalters-edited commented Feb 27, 2024

fmcwalters-edited commented Feb 28, 2024

gchazot left a comment

Choose a reason for hiding this comment

gchazot Mar 11, 2024

Choose a reason for hiding this comment

gchazot Mar 11, 2024

Choose a reason for hiding this comment

gchazot Mar 11, 2024

Choose a reason for hiding this comment

gchazot Mar 11, 2024

Choose a reason for hiding this comment

gchazot Mar 11, 2024

Choose a reason for hiding this comment

gchazot Mar 11, 2024

Choose a reason for hiding this comment

gchazot Mar 11, 2024

Choose a reason for hiding this comment

fmcwalters-edited commented Feb 15, 2024 •

edited

Loading

fmcwalters-edited commented Feb 16, 2024 •

edited

Loading