New Docker+Buildkit client #2327
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The container log output format depends on wether tty is enabled or not. - when enabled logs are a single stream from stdout - when not enabled stdout and stderr streams are multiplexed into a single stream with 8 byte headers on each line
93a8844 to
238d053
Compare
Signed-off-by: Michael Dwan <m@dwan.io>
…o md/docker-api-client-1
helpers to map errors from different backends to known types. needed to support both clients
Signed-off-by: Michael Dwan <m@dwan.io>
There was a problem hiding this comment.
Pull Request Overview
Adds a new Docker SDK–based client alongside the existing CLI client and wires it through tests and CI.
- Introduce
TestDockerA 10000 PIClientto validate the new SDK client in tests - Switch
NewClientto return the SDK client when enabled - Add BuildKit integration (
buildkit.go+build_secrets.go) and registry auth loading - Update CI to run both clients via a matrix on
COG_DOCKER_SDK_CLIENT
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/docker/docker_client_test.go | Add TestDockerAPIClient for SDK client tests |
| pkg/docker/docker.go | Return SDK client in NewClient instead of panic |
| pkg/docker/credentials.go | Load Docker registry auth via SDK types |
| pkg/docker/command/command.go | Add Secrets and BuildArgs to build options |
| pkg/docker/buildkit.go | New BuildKit–based implementation |
| pkg/docker/build_secrets.go | Parse BuildKit secrets inputs |
| go.mod | Pull in BuildKit, gRPC, and CSV-value deps |
| .github/workflows/ci.yaml | Add new_docker_api_client matrix for CI runs |
Comments suppressed due to low confidence (1)
pkg/docker/command/command.go:32
- [nitpick] Either perform the rename now or add a clear migration path; leaving mismatched field names can cause confusion.
// TODO[md]: ImageName should be renamed to Tag
Signed-off-by: Michael Dwan <m@dwan.io>
8W9aG
approved these changes
May 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #2344merged!Now depends on #2353
This PR adds a new
command.Commandimplementation (gonna rename that todocker.Clientsoon) that interacts with backends using the docker client and buildkit client packages. Although there's no meaningful end user improvements here beyond bug fixes and a modest ~10% build & predict performance boost, this change exposes the low-level APIs we need for upcoming work on high performance model packaging and delivery.The existing client will remain the default for now, and this new one can be enabled with
COG_DOCKER_SDK_CLIENT=1. The integration tests are currently testing both clients in parallel. Once we're feeling good about it we can make it default and rip the old one out.