feat: add support for using CLI container clients#8661
feat: add support for using CLI container clients#8661reedham-aws merged 16 commits intoaws:feat-buildkitfrom
Conversation
This change also refactors a lot of docker specific things to be "container_client" rather than "docker_client". In addition, moving from instantiating the image build client in build context to lazily doing it in the app builder.
| result = subprocess.run( | ||
| ["docker", "buildx", "version"], | ||
| capture_output=True, | ||
| check=True, |
There was a problem hiding this comment.
If docker buildx version fails, it raises CalledProcessError before the returncode != 0 check is ever reached. This should use check=False or wrap in a try/except for CalledProcessError.
The Finch path has the same issue.
There was a problem hiding this comment.
I changed this to be check=False. I originally had let it fall through to the if result.returncode != 0, but then ruff didn't like the lack of explicit check. After that I forgot about the original intention 🙃.
| process.wait() | ||
|
|
||
| if process.returncode != 0: | ||
| yield {"error": f"Build failed with exit code {process.returncode}"} |
There was a problem hiding this comment.
This does not raise an exception. The caller function expects docker.errors.BuildError on build failure.
There was a problem hiding this comment.
Good callout. I changed this to raise the error instead of stream like this. What I did above was capture the stdout and save it in a list called build_log, then pass that as the build_log for the docker.errors.BuildError, which has the following definition:
class BuildError(DockerException):
def __init__(self, reason, build_log):
super().__init__(reason)
self.msg = reason
self.build_log = build_logI think this should be fine, although I guess if the logs are long enough memory usage could balloon. Let me know if you think that is an issue.
* feat: add support for using CLI container clients (#8661) * feat: add build_client * feat: add --use-buildkit option * feat: use build client when building with images * lint and format * feat: add command to cli * fix: use absolute path for dockerfile * test: add unit test for build_client * test: add unit test for build_context * fix: additional test fixes * fix: schema update * fix: remove hardcoded unix pathseps * fix: remove provenance and sbom from docker buildx * fix: refactor code to use SDKBuildClient This change also refactors a lot of docker specific things to be "container_client" rather than "docker_client". In addition, moving from instantiating the image build client in build context to lazily doing it in the app builder. * test: add additional unit tests * fix: fix error handling in CLIBuildClient * fix: reorder error handling in CLIImageBuilder * test: add integration tests for CLI client (#8689) * test: add basic integration test * test: more comprehensive integ tests * test: add arm64 test and mark as tier1 * fix: update dockerfile error messages * test: remove arm64 test * fix: allow finch without sudo
Which issue(s) does this change fix?
#3972, #8656, #3852
Why is this change necessary?
Currently, the docker-py SDK does not support BuildKit: docker/docker-py#2230. This means that in order for
sam buildto use BuildKit, we must use the Docker (or Finch) CLI.How does it address the issue?
This change adds a
BuildClientclass that acts as a parent to either theSDKBuildClientor theCLIBuildClient. If a user passes asam buildargument of--use-buildkit, theCLIBuildClientis used with eitherdocker buildx buildorfinch build ...(Finch uses buildkit by default).What side effects does this change have?
For existing builds, there shouldn't be any. Using the CLI is entirely opt in, and the SDK build is doing the same thing as before with only a thin layer in between. There could be a difference between the SDK build and the CLI build for different applications (especially for things like cross platform builds, which buildkit is for). There also isn't any change to the detection of which build engine to use, so it shouldn't change where Docker/Finch is used.
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.