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.
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.