-
Notifications
You must be signed in to change notification settings - Fork 405
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
Allow moby-compose and docker-compose-plugin versions to be specified in devcontainer.json for Docker in Docker #841
base: main
Are you sure you want to change the base?
Conversation
… in devcontainer.json This changeset allows users to specify the versions of the docker-compose-plugin and moby-compose packages to be installed as part of the docker-in-docker devcontainer feature. This is provided as a way to allow users to pin the underlying versions of these packages in cases where newer versions may not be working or available Also included in this changeset is a refactor of the way package versions are discovered to ensure the process is consistent and DRY
@samruddhikhandale Per our earlier conversation, here's a starting place for a package-installation refactor. This also applies the same configuration to I haven't tested this PR yet, so I'm curious to see what the workflow run says about it. Feel free to make changes to this PR yourself or request changes from me - I mostly put this together to scratch my own itch :) |
cli_version_suffix=get_package_version_suffix "${cli_package_name}" "${DOCKER_VERSION}" | ||
required_packages="${cli_package_name}${cli_version_suffix} ${engine_package_name}${engine_version_suffix}" | ||
|
||
# Moby always uses buildx |
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.
Is this true?
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.
I don't think so!
Reasoning :buildx
is a Docker CLI plugin that extends the docker build command with the full support of the features provided by Moby BuildKit builder toolkit. However, buildx
is not the default build engine for Docker or Moby. The default builder in Docker is still the legacy docker build
command. To use buildx
, you need to either install it as a Docker CLI plugin or use a version of Docker that has buildx bundled with it (Docker 19.03 and later versions).
So, while Moby and Docker can use buildx
for building images, it's not always used and depends on the specific configuration and version of Docker you're using.
(Feel free to correct if I have misunderstood)
|
||
if [ -z "${package_version_suffix}" ] || [ "${package_version_suffix}" = "=" ]; then | ||
err "No full or partial ${package_name} version match found for \"${package_version}\" on OS ${ID} ${VERSION_CODENAME} (${architecture}). Available versions:" | ||
apt-cache madison ${package_name} | awk -F"|" '{print $2}' | grep -oP '^(.+:)?\K.+' |
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.
Will this still echo to the log from within this function, or does it need to be redirected to stderr?
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.
Pretty cool refactoring, thank you so much 🎉
Left some thoughts.
@@ -50,6 +45,16 @@ | |||
"type": "boolean", | |||
"default": true, | |||
"description": "Install Docker Buildx" | |||
}, | |||
"mobyBuildxVersion": { |
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.
Can we add test scenarios to validate/massage these two options? thanks!
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.
Sure, will do
local package_version_regex="^(.+:)?${package_version_dot_plus_escaped}([\\.\\+ ~:-]|$)" | ||
|
||
set +e # Catch errors so we can show more useful output | ||
local package_version_suffix="=$(apt-cache madison ${package_name} | awk -F"|" '{print $2}' | sed -e 's/^[ \t]*//' | grep -E -m 1 "${package_version_regex}")" |
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.
Nit: Can we indent all the code in between set
commands for better code readability?
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.
Sure thing. I typically don't consider set +e
a scope, so I find the style a little confusing personally. Is there a bash styleguide this repo follows?
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.
Is there a bash styleguide this repo follows?
I don't think there's a specific guide that we follow for this repo, trying to follow the historical stylings of these files (as well as happy to learn some new and standard styles) 😬
apt-get -y install --no-install-recommends docker-compose-plugin || echo "(*) Package docker-compose-plugin (Docker Compose v2) not available for OS ${ID} ${VERSION_CODENAME} (${architecture}). Skipping." | ||
# Install required packages (engine, CLI, and moby-buildx if moby) | ||
set +e | ||
apt-get -y install --no-install-recommends ${required_packages} |
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.
Same indent comment here..
Co-authored-by: Samruddhi Khandale <[email protected]>
This changeset allows users to specify the versions of the docker-compose-plugin and moby-compose packages to be installed as part of the docker-in-docker devcontainer feature. This is provided as a way to allow users to pin the underlying versions of these packages in cases where newer versions may not be working or available
Also included in this changeset is a refactor of the way package versions are discovered to ensure the process is consistent and DRY
(Motivations for this change in #838 + #837)