-
Notifications
You must be signed in to change notification settings - Fork 846
Avoid installing missing packages for version 23 (and moving them to 28.2) #511
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
Avoid installing missing packages for version 23 (and moving them to 28.2) #511
Conversation
vvoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Left some comments
install.sh
Outdated
| fi | ||
| if version_gte "23.0"; then | ||
| pkgs="$pkgs docker-buildx-plugin docker-model-plugin" | ||
| case "$lsb_dist.$dist_version" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be inside the RPM-based block, it needs to go in:
Lines 549 to 558 in 7040dd2
| if version_gte "18.09"; then | |
| # older versions didn't ship the cli and containerd as separate packages | |
| pkgs="$pkgs docker-ce-cli${cli_pkg_version%=} containerd.io" | |
| fi | |
| if version_gte "20.10"; then | |
| pkgs="$pkgs docker-compose-plugin docker-ce-rootless-extras$pkg_version" | |
| fi | |
| if version_gte "23.0"; then | |
| pkgs="$pkgs docker-buildx-plugin docker-model-plugin" | |
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vvoland I just move it (from line 600-something to line 557). But I can also use your "version 28" approach if you want me to.
install.sh
Outdated
| if version_gte "23.0"; then | ||
| pkgs="$pkgs docker-buildx-plugin docker-model-plugin" | ||
| case "$lsb_dist.$dist_version" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I think we can make our life easier and only consider model plugin from 28.0+ up. (buildx still 23.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @doringeman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vvoland I agree, ok, so would you like me to just replace the "new" switch case by just an additional if version_gte "28.0" that contains just the model plugin (while keeping just the buildx plugin in the "if 23" section)? I could do that. Not sure if v28 is availble for me to test right now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct!
For testing purposes, you can use whatever version_gte X you want, just make sure to change it before pushing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vvoland Ok I did just that, and I used @thaJeztah suggestion of version 27 (but if you would rather use version 28 I can do that too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. This is a best-effort check anyway 👍🏻
install.sh
Outdated
| fi | ||
| if version_gte "23.0"; then | ||
| pkgs="$pkgs docker-buildx-plugin docker-model-plugin" | ||
| case "$lsb_dist.$dist_version" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should avoid having to maintain this list of distros. We definitely don't want to end up having adding each new distro to the list.
We should default to installing all plugins (after a version check), and then only disable for the distros we know that are not supported.
|
I'm a bit on the fence on this; we're adding complexity for EOL distros that the script no longer maintains; perhaps instead we need to make the deprecation warning a hard failure instead; curl -fsSL https://get.docker.com | DRY_RUN=1 sh
# Executing docker install script, commit: 7040dd2bf115a359317b1de84de611aeabcb7bc2
DEPRECATION WARNING
This Linux distribution (ubuntu bionic) reached end-of-life and is no longer supported by this script.
No updates or security fixes will be released for this distribution, and users are recommended
to upgrade to a currently maintained version of ubuntu.
Press Ctrl+C now to abort this script, or wait for the installation to continue.
^CAlternatively, we could make the model plugin conditional on version, and only install on current version(s) (e.g., 27.x and up), although strictly speaking, the plugin doesn't depend on those versions, and could be installed on older versions; Lines 658 to 660 in 7040dd2
|
3214c19 to
e856a4f
Compare
vvoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after DCO
e856a4f to
2b7768e
Compare
|
I found an easy way to test if this works using docker containers (instead of EC2 machines or other VM types): output: So yeah without a --version, the script still fails (so older CI/CD pipelines will still have to be adjusted to add a --version statement). But with the command: output: Now this works fine. So it looks like this PR modification has the intended effect to only install docker-buildx-plugin (not docker-model-plugin anymore) when specifying --version 23.0 with success. I also tested with --version 27.0 but obviously that is not yet available of course: I will test the other distros too later today (using the same docker commands). Just brainstorming here: could we put that test in an automated github CI/CD pipeline? Maybe that would be too costly. |
|
So I tested with: Then I tested with ubuntu:22.04 and it also works great. Then I tested with ubuntu:24.04 and it also works great. Lastly, I thought I would test ubuntu:20.04 and I realised: there is a small problem that we could address. This distro version 20.04 supports docker version 27, even version 28, but does not contain the docker-model-plugin associated with it. It looks like the docker-model-plugin was released with version 28.2 (and ubuntu:20.04 stopped having new docker versions at 28.1). So maybe I could change the if condition to version_gte 28.2 to support ubuntu:20.04 too, what do you think @vvoland? Would that be overkill? |
|
It's fine IMO. It's only a default anyway, and user can always do explicit installation of the model plugin. |
|
Yes, changing to I guess it's fine to just do it by default on v28.2 (or "latest") as installing "latest version" would be the most common case for users to install using this script |
On ubuntu bionic and focal (and debian buster too), the docker-model-plugin package is missing. The suggestion was to just install the package named docker-buildx-plugin in the version 23 check, while moving the missing package docker-model-plugin to a new version 28.2 check. The number was chosen because that package becomes available starting with version 28.2 on ubuntu jammy (while not available in version 28.1 on ubuntu focal). See: docker#509 Signed-off-by: Rejean Groleau <rejeangroleau@gmail.com>
2b7768e to
4ff191b
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
@vvoland PTAL |
vvoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
On ubuntu bionic and some other debian/versions, the docker-model-plugin package is missing. The suggestion was to just install the package named docker-buildx-plugin in the version 23 check, while moving the missing package docker-model-plugin to a new "version 28.2" check.
See: #509