v2.1 Add ability to version installations of docker-ce and docker-ce-cli #5
Conversation
gchazot
left a comment
gchazot
left a comment
There was a problem hiding this comment.
Couple of suggestions to make it more user-friendly.
Something in the README or the example would also help with that a lot.
pyinfra_docker/docker.py
Outdated
| def get_pkgs_to_install(version): | ||
| docker_packages = [ | ||
| "docker-ce", | ||
| "docker-ce-cli" |
There was a problem hiding this comment.
Probably worth adding a flag to choose whether to install the CLI package, defaulting to False since this was the behaviour before this change.
There was a problem hiding this comment.
I did think about this but that's not true, docker-ce-cli is a dependancy of docker-ce but it looks like docker isn't dependancy managing correctly as docker-ce breaks with docker-ce-cli=>23.0 (from the testing I've done). But just pinning docker-ce installs 23.0 for the cli.
There are other dependancies which didn't give any problems but follow a different versioning so shouldn't be tied to this. docker-ce-rootless-extras seems to follow the same versioning and is part of the docker-ce ecosystem so it might make sense to pin that one as well...
$ sudo apt-get install docker-ce=5:20.10.23~3-0~ubuntu-focal
.....
The following additional packages will be installed:
containerd.io docker-buildx-plugin docker-ce-cli docker-ce-rootless-extras docker-compose-plugin docker-scan-plugin pigz slirp4netns
Suggested packages:
aufs-tools cgroupfs-mount | cgroup-lite
The following NEW packages will be installed:
containerd.io docker-buildx-plugin docker-ce docker-ce-cli docker-ce-rootless-extras docker-compose-plugin docker-scan-plugin pigz slirp4netns
.....
Do you want to continue? [Y/n] y
Get:1 https://mirror.hetzner.com/ubuntu/packages focal/universe amd64 pigz amd64 2.4-1 [57.4 kB]
Get:2 https://mirror.hetzner.com/ubuntu/packages focal/universe amd64 slirp4netns amd64 0.4.3-1 [74.3 kB]
Get:3 https://download.docker.com/linux/ubuntu focal/stable amd64 containerd.io amd64 1.6.16-1 [27.7 MB]
Get:4 https://download.docker.com/linux/ubuntu focal/stable amd64 docker-buildx-plugin amd64 0.10.2-1~ubuntu.20.04~focal [25.9 MB]
Get:5 https://download.docker.com/linux/ubuntu focal/stable amd64 docker-ce-cli amd64 5:23.0.0-1~ubuntu.20.04~focal [13.2 MB]
Get:6 https://download.docker.com/linux/ubuntu focal/stable amd64 docker-ce amd64 5:20.10.23~3-0~ubuntu-focal [20.5 MB]
Get:7 https://download.docker.com/linux/ubuntu focal/stable amd64 docker-ce-rootless-extras amd64 5:23.0.0-1~ubuntu.20.04~focal [8,765 kB]
Get:8 https://download.docker.com/linux/ubuntu focal/stable amd64 docker-compose-plugin amd64 2.15.1-1~ubuntu.20.04~focal [9,572 kB]
Get:9 https://download.docker.com/linux/ubuntu focal/stable amd64 docker-scan-plugin amd64 0.23.0~ubuntu-focal [3,622 kB]
pyinfra_docker/docker.py
Outdated
| versioned_packages = [] | ||
| for pkg in docker_packages: | ||
| versioned_packages.append(f"{pkg}={version}") | ||
|
|
||
| return versioned_packages |
There was a problem hiding this comment.
| versioned_packages = [] | |
| for pkg in docker_packages: | |
| versioned_packages.append(f"{pkg}={version}") | |
| return versioned_packages | |
| return [f"{pkg}={version}" for pkg in docker_packages] |
pyinfra_docker/docker.py
Outdated
| def deploy_docker(config=None, version=None): | ||
| """ | ||
| Install Docker on the target machine. | ||
| Args: | ||
| config: filename or dict of JSON data | ||
| """ | ||
|
|
||
| packages = get_pkgs_to_install(version) |
There was a problem hiding this comment.
What about making version a host data, maybe with default, instead?
There was a problem hiding this comment.
I don't think we want to force the default to be an older package version for now, while it's having issues for us it might be very swiftly patched and anyone using this will be getting an older package 🙃
There was a problem hiding this comment.
The default could be None and the same logic then applies. It would remove the need to pass the version through the functions.
I'm not sure this is the best coding practice (prefer dependency injection over global variables) but it seems to me it would be more in line with the spirit of pyinfra.
There was a problem hiding this comment.
I'm not sure this is the best coding practice (prefer dependency injection over global variables) but it seems to me it would be more in line with the spirit of pyinfra.
heh, totally agree with this, the deploy data defaults thing is... not great. But absolutely right thing to do here now.
Fizzadar
left a comment
Fizzadar
left a comment
There was a problem hiding this comment.
LGTM aside from centOS package version fix!
pyinfra_docker/docker.py
Outdated
| if not host.data.docker_version: | ||
| return docker_packages | ||
|
|
||
| return [f"{pkg}={host.data.docker_version}" for pkg in docker_packages] |
There was a problem hiding this comment.
This will need to use - to join name+version under yum/dnf (https://unix.stackexchange.com/questions/151689/how-can-i-instruct-yum-to-install-a-specific-version-of-package-x).
Co-authored-by: Germain Chazot <g.chazot@gmail.com>
gchazot
left a comment
gchazot
left a comment
There was a problem hiding this comment.
I've tested locally on Ubuntu/Debian and Centos/Almalinux (using the deploy in example) and it's all working nicely.
|
I've now also created a draft PR: #6 |
|
Apologies for the delay guys, busy times! Thanks for the changes 🙏, LGTM! |
3 participants
@Fizzadar Currently tested on ubuntu, haven't got round to testing on CentOS yet.