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

libcnb-test: Migrate from Bollard to Docker CLI #621

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Jul 31, 2023

Previously libcnb-test ran its various Docker related actions/commands (excluding those handled by Pack CLI) using the bollard crate.

Whilst at first glance using a Rust client for Docker seems preferable, it turns out that using Bollard/the Docker daemon API actually has a number of disadvantages, which are explained in more detail in #620.

Now:

  • The Docker CLI is used instead of Bollard+Tokio+the Docker daemon API, which is simpler to understand/maintain and also avoids ~55 additional transitive dependencies.
  • The run_shell_command and shell_exec commands are now run as attached docker run invocations, making it trivial to check their exit code, fixing libcnb-test: Exit status of commands run via run_shell_command and shell_exec are not checked #446.
  • Whenever errors occur, the error output is now easier for end users to understand, since it's presented as Docker CLI output with which they will be more familiar (vs Docker daemon/Bollard API errors).

As part of this change, ContainerConfig::entrypoint can no longer accept a vector of strings, since the Docker CLI's --entrypoint arg only accepts a single value, unlike the Docker daemon API. However, since the purpose of libcnb-test is to replicate typical end-user usage of the buildpacks and resultant images, this actually improves alignment of the framework with the use-cases we want to test.

Fixes #446.
Closes #620.
GUS-W-11382688.
GUS-W-13853580.

@edmorley edmorley added bug Something isn't working enhancement New feature or request libcnb-test breaking change labels Jul 31, 2023
@edmorley edmorley self-assigned this Jul 31, 2023
Previously libcnb-test ran its various Docker related actions/commands
(excluding those handled by Pack CLI) using the bollard crate.

Whilst at first glance using a Rust client for Docker seems preferable,
using Bollard/the Docker daemon API actually has a number of
disadvantages, which are explained in more detail here:
#620

Now:
- Docker CLI is used instead of Bollard/the Docker daemon API, which is
  simpler to understand/maintain and also avoids ~55 additional
  transitive dependencies.
- The `run_shell_command` and `shell_exec` commands are now run as
  attached `docker run` invocations, making it trivial to check
  their exit code, fixing #446.
- Whenever errors occur, the error output is now easier for end users
  to understand, since it's presented as Docker CLI output with which
  they will be more familiar (vs Docker daemon/Bollard API errors).

As part of this change, `ContainerConfig::entrypoint` can no longer
accept a vector of strings, since the Docker CLI's `--entrypoint` arg
only accepts a single value, unlike the Docker daemon API. However,
since the purpose of `libcnb-test` is to replicate typical end-user
usage of the buildpacks and resultant images, this actually improves
alignment of the framework with the use-cases we want to test.

Fixes #446.
Closes #620.
GUS-W-11382688.
GUS-W-13853580.
@edmorley edmorley marked this pull request as ready for review July 31, 2023 14:37
@edmorley edmorley requested a review from a team as a code owner July 31, 2023 14:37
Copy link
Member

@joshwlewis joshwlewis left a comment

Choose a reason for hiding this comment

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

This changes the nature of the docker dependency a bit - bollard was tightly constrained, and the docker daemon API is fairly stable. Now we're loosely dependent on a docker CLI, of which users may have varying versions. That might mean subtle usage changes that are incompatible with this implementation. Should we document the version (or range) we're targeting? Or maybe codify the installation of docker so we catch docker CLI changes when they happen?

(These suggestions are optional - I'm very happy to know about non-zero exit codes)

@edmorley
Copy link
Member Author

@joshwlewis Yeah I agree it's something we should keep an eye on longer term. I think in practice it hopefully shouldn't be a problem - none of the Docker command arguments we're using are particularly unusual, and I'm pretty sure have been supported by the Docker CLI since the beginning of time. I guess we already have the same issue in theory with Pack CLI versions.

Should we document the version (or range) we're targeting?

I don't know what versions we could put - there's not an easy way for me to find out. On my local machine I'm using Docker 24.0.2 and GitHub Actions has 20.10.25 pre-installed, but other than that your guess is as good as mine :-)

Or maybe codify the installation of docker so we catch docker CLI changes when they happen?

I don't think libcnb-test should be responsible for installing Docker (or that there would even be a sensible way to implement). The libcnb-test README mentions Docker needs to be installed - since Pack CLI needs it already:
https://github.com/heroku/libcnb.rs/blob/main/libcnb-test/README.md#dependencies

If we ever find out that a specific old (or new) version of Docker doesn't work, we can document it in the README and/or add a version check in libcnb-test. (Same if we have version issues with Pack CLI.)

@edmorley edmorley merged commit 9aef71d into main Aug 1, 2023
4 checks passed
@edmorley edmorley deleted the edmorley/rm-bollard branch August 1, 2023 09:15
@edmorley edmorley added the faster compiles Things that improve compile times label Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug Something isn't working enhancement New feature or request faster compiles Things that improve compile times libcnb-test
Projects
None yet
3 participants