Supporting changes for new docker client #2344
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 credential stuff isn't tied to the docker client. This moves logic to helper functions that both clients can use
The monobase helpers from command.Command were calling docker run, instead have them call the run functions on the client since the logic doesn't depend on client implementation
helpers to map errors from different backends to known types. needed to support both clients
There was a problem hiding this comment.
Pull Request Overview
Adds shared helpers and refactors the Docker client implementation and tests to support a new unified client interface, plus an integration test for build secrets.
- Extracted common tar-creation and credential logic into
pkg/docker/monobase.goandpkg/docker/credentials.go - Enhanced the registry test helper (
StartTestRegistry) with auth options, repo cloning, and image-exists checks - Updated unit and integration tests (including a new secrets fixture) to use the new helper functions
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test-integration/.../predict.py | Added sample Predictor for secrets integration tests |
| test-integration/.../file-secret.txt | Added a file-based secret value |
| test-integration/.../cog.yaml | Defined mounts and assertions for file and env secrets |
| pkg/registry_testhelpers/registry_container.go | Made StartTestRegistry configurable, added auth, clone, exists helpers |
| pkg/docker/monobase.go | Extracted CreateTarFile/CreateAptTarFile implementations |
| pkg/docker/credentials.go | Factored out credential loading into standalone functions |
| pkg/docker/docker_command.go | Updated Push and LoadUserInformation to use new helpers |
| pkg/docker/command/command.go | Removed tar helpers from the Command interface |
| pkg/docker/dockertest/* | Refactored test helpers and mocks to drive new client tests |
| pkg/docker/docker_client_test.go | Rewrote client tests to leverage new registry and helper APIs |
| go.mod | Added golang.org/x/crypto dependency |
Comments suppressed due to low confidence (5)
test-integration/test_integration/fixtures/secrets-project/cog.yaml:14
- The second secret mount uses
id: env-secretbut no corresponding secret source or environment variable is defined in the fixtures; this test will fail at runtime. Consider adding a file forenv-secretor specifying an env var to back it.
- - type: secret
pkg/registry_testhelpers/registry_container.go:142
- [nitpick] The
WithAuthfunction returns an anonymousfunc(*options)but the type alias is namedOption. Consider changing the signature tofunc WithAuth(...) Optionfor clarity.
func WithAuth(username, password string) func(*options) {
pkg/docker/docker_command.go:9
- After removing the old
CreateTarFileandCreateAptTarFilemethods, there may be unused imports in this file. Rungo mod tidy/goimportsto clean them up.
import (
pkg/docker/command/command.go:15
- The interface no longer declares
CreateTarFileorCreateAptTarFile, but implementations have been moved out. Consider removing any dead methods onDockerCommandor elsewhere to keep the interface and implementations in sync.
type Command interface {
pkg/docker/monobase.go:22
- This function returns only the tar filename, while
CreateTarFilereturns the full path. It may be more consistent to return the absolute path here as well.
func CreateAptTarball(ctx context.Context, tmpDir string, dockerClient command.Command, packages ...string) (string, error) {
8W9aG
approved these changes
May 19, 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.
These are changes to the existing codebase extracted from #2327 to make the diff easier to review.
No behavior changes expected. Each commit is roughly one logical change, though the diff might be strange because I did a half ass job of splitting them up to save time. The end result is the same though.
The main things:
Several functions on
command.Commanddidn't actually change between the cli and sdk clients. So I extracted them out to helper functions that operate on acommand.Command.command.Command. I also moved related code tomonobase.gofile to make it clear what that stuff is forA few other things:
Once this is merged I can tidy up #2327 for review 🎉