-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
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.
ae457bd
to
dcbdd48
Compare
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 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)
@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.
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 :-)
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: 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.) |
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:
run_shell_command
andshell_exec
commands are now run as attacheddocker run
invocations, making it trivial to check their exit code, fixing libcnb-test: Exit status of commands run viarun_shell_command
andshell_exec
are not checked #446.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 oflibcnb-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.