Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sayhiben
Copy link
Contributor

@sayhiben sayhiben commented Feb 10, 2024

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)

… 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
@sayhiben sayhiben requested a review from a team as a code owner February 10, 2024 04:05
@sayhiben
Copy link
Contributor Author

@samruddhikhandale Per our earlier conversation, here's a starting place for a package-installation refactor. This also applies the same configuration to moby-compose and docker-compose-plugin we merged earlier, so I've bumped the minor version number again

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 :)

@sayhiben sayhiben changed the title Allow moby-compose and docker-compose-plugin versions to be specified in devcontainer.json Allow moby-compose and docker-compose-plugin versions to be specified in devcontainer.json for Docker in Docker Feb 10, 2024
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true?

Copy link
Member

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.+'
Copy link
Contributor Author

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?

Copy link
Member

@samruddhikhandale samruddhikhandale left a 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.

src/docker-in-docker/install.sh Outdated Show resolved Hide resolved
@@ -50,6 +45,16 @@
"type": "boolean",
"default": true,
"description": "Install Docker Buildx"
},
"mobyBuildxVersion": {
Copy link
Member

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!

Copy link
Contributor Author

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}")"
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same indent comment here..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants