Replace docker.Xxx helpers with functions on command.Command
#2288
Conversation
8ff9f08 to
9bf34df
Compare
DockerCommand.exec will write the underlying command stdout and stderr to the provided io.Writer. This allows callers to either capture the entire output or allow long running commands to stream output to a writer that isn't os.Stdout or os.Stderr
9bf34df to
49d396a
Compare
github.com/docker/docker needed an upgrade, but this wasn't compatible with a dependency of goreleaser, so I removed it from go tools
…ation Several helper functions in the `docker` package (`ImageInspect`, `ImageExists`, `ContainerInspect`, `ContainerLogsFollow`, and `Pull`) invisibly shelled out to the docker CLI, often with slightly different logic than was the `DockerCommand` struct was doing. This PR moves them to functions on the `DockerCommand` struct as well as adds them to the `command.Command` interface so they can be swapped out with the upcoming engine client.
docker.Xxx helpers with functions on command.Commanddocker.Xxx helpers with functions on command.Command
test harness for running integration tests against an actual docker daemon and local registry
different errors are returned when working with a docker registry and OCI registry. This maps both of them to the not found error
56ffb42 to
6f688f9
Compare
6f688f9 to
60ac3a7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR migrates legacy docker helper functions into methods on the command.Command interface and updates their usage throughout the codebase. Key changes include:
- Consolidating docker CLI calls into the DockerCommand type and updating the Command interface.
- Removing legacy functions (e.g. Stop, Pull, Logs, Inspect, ImageExists, ContainerInspect) from the docker package.
- Updating usage in the CLI commands (train and predict) and test suites to leverage the new DockerCommand methods.
Reviewed Changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/image/openapi_schema.go | Updated GetOpenAPISchema to use a dockerClient parameter and manifest data. |
| pkg/image/config.go | Renamed config function and updated error messages with friendly names. |
| pkg/docker/stop.go, pkg/docker/pull.go, pkg/docker/logs.go, pkg/docker/image_inspect.go, pkg/docker/image_exists.go | Removed legacy implementations. |
| pkg/docker/dockertest/mock_command.go | Updated Inspect signature and added stubs for other methods. |
| pkg/docker/docker_command.go | Consolidated docker CLI commands into DockerCommand methods with enhanced debug logging and error wrapping. |
| pkg/docker/command/errors.go & pkg/docker/command/command.go | Introduced NotFoundError and updated the Command interface methods. |
| pkg/cli/train.go & pkg/cli/predict.go | Updated to use DockerCommand; integrated error wrapping for image inspection. |
| pkg/docker/docker_client_test.go | Updated tests to work with the new DockerCommand implementations. |
Files not reviewed (3)
- Makefile: Language not supported
- go.mod: Language not supported
- pkg/docker/dockertest/testdata/create-image-fixtures.sh: Language not supported
pkg/cli/predict.go
Outdated
| @@ -112,17 +115,18 @@ func cmdPredict(cmd *cobra.Command, args []string) error { | |||
| return fmt.Errorf("Invalid image name '%s'. Did you forget `-i`?", imageName) | |||
| } | |||
|
|
|||
| exists, err := docker.ImageExists(ctx, imageName) | |||
| manifest, err := dockerCommand.Inspect(ctx, imageName) | |||
There was a problem hiding this comment.
Is there a quicker way to get an existance check than an inspect call here?
There was a problem hiding this comment.
Yes! command.Command.ImageExists.
Though I just pushed up a fix to the copilot issue that added an inspect step to pull that'll replace the need to do repeated inspect calls. Now pull will always return an inspect response, and there's an option to either use it or fetch and return the updated inspect response via the force flag.
There was a problem hiding this comment.
just realized you reviewed before I pushed up the changes to Pull that I mentioned. Here's the commit if you're curious: 9e0b967
There's a few places in the code where we check for a local image, and if not found begin a pull, then carry on. I had a bug that allowed pull failures to be swallowed and stale inspect responses to be passed to predict+train. This fixes the issue by ensuring that Pull returns the inspect response of the image that was just pulled, or to skip pulling if the image already exists using a force flag.
b00bae0 to
50adc50
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors Docker CLI helper functions by migrating them to methods on the command.Command interface, while consolidating operations (Inspect, ContainerInspect, ContainerStop, ContainerLogs, and Pull) into a unified client. Key changes include replacing direct calls to docker.Xxx helpers with command.Command methods, updating method signatures to return enriched responses, and removing legacy docker package files.
Reviewed Changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/image/config.go | Updates GetConfig to CogConfigFromManifest and error message tweaks |
| pkg/dockerfile/fast_generator.go | Adjusted Pull call to include a force flag and expect an inspect response |
| pkg/docker/* (various files) | Removed legacy docker helper files, consolidating functionality |
| pkg/docker/dockertest/mock_command.go | Updated mock interface with new signatures and non-implemented stubs |
| pkg/docker/docker_command.go | Refactored methods (Pull, Inspect, ContainerStop, etc.) with enhanced logging and error handling |
| pkg/cli/train.go & pkg/cli/predict.go | Updated usage of docker client methods to align with new signatures |
Files not reviewed (3)
- Makefile: Language not supported
- go.mod: Language not supported
- pkg/docker/dockertest/testdata/create-image-fixtures.sh: Language not supported
Comments suppressed due to low confidence (1)
pkg/docker/dockertest/mock_command.go:81
- [nitpick] In the mock implementations, instead of using panic for unimplemented methods (e.g. ImageExists, ContainerLogs, etc.), consider returning a descriptive error to avoid abrupt test failures if these paths are inadvertently invoked.
func (c *MockCommand) ImageExists(ctx context.Context, ref string) (bool, error) { panic("not implemented") }
| // TODO[md]: find the tag/ref and return that in the error instead of the ID | ||
| return nil, fmt.Errorf("Image %s does not appear to be a Cog model", friendlyName(manifest)) | ||
| } | ||
| conf := new(config.Config) | ||
| if err := json.Unmarshal([]byte(configString), conf); err != nil { | ||
| return nil, fmt.Errorf("Failed to parse config from %s: %w", imageName, err) | ||
| // TODO[md]: find the tag/ref and return that in the error instead of the ID | ||
| return nil, fmt.Errorf("Failed to parse config from %s: %w", friendlyName(manifest), err) | ||
| } | ||
| return conf, nil | ||
| } | ||
|
|
||
| func friendlyName(manifest *image.InspectResponse) string { | ||
| // this appears to get the base image name, which we don't really want | ||
| // name := manifest.Config.Labels["org.opencontainers.image.title"] | ||
| // if name != "" { | ||
| // return name | ||
| // } | ||
|
|
||
| if len(manifest.RepoTags) > 0 { | ||
| return manifest.RepoTags[0] | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider updating the error message to include a more descriptive tag or reference (instead of relying solely on friendlyName) once that information is available. Removing the TODO comments when this is addressed will improve clarity in error reporting.
Copilot uses AI. Check for mistakes.
| return "", err | ||
| } | ||
|
|
||
| return aptTarFile, nil | ||
| } | ||
|
|
||
| func (c *DockerCommand) Inspect(ctx context.Context, image string) (*command.Manifest, error) { | ||
| func (c *DockerCommand) Inspect(ctx context.Context, ref string) (*image.InspectResponse, error) { | ||
| console.Debugf("=== DockerCommand.Inspect %s", ref) |
There was a problem hiding this comment.
[nitpick] The Inspect method logs the debug information using a nearly identical message both before executing and after capturing output, which could be confusing. Consider differentiating the log messages (for example, one for the input reference and another for the returned output) to improve traceability.
| console.Debugf("=== DockerCommand.Inspect %s", ref) | |
| console.Debugf("=== DockerCommand.Inspect: Inspecting reference %s", ref) |
Copilot uses AI. Check for mistakes.
The TLDR is:
docker.DockerClientstructcommand.Commandinterface (which'll get renamed to something likedocker.Clientshortly) so I can add the direct engine client without changing build logicgithub.com/docker/dockerandgithub.com/docker/clipackages. Unfortunately there was an irreconcilable conflict with a far flung goreleaser dependency, so I removed it fromgo tools.Since this one is big (though mostly benign) I split each logical chunk into a commit for easier review.
This PR moves the following operations to
command.Command:I'll knock the chonky ones out in subsequent PRs, like Build and Run.