-
Notifications
You must be signed in to change notification settings - Fork 641
move docker.Build to command.Command
#2293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the build process by moving the docker.Build functionality into the command.Command interface and updating all callers accordingly. Key changes include:
- Replacing docker.Build calls with dockerCommand.ImageBuild calls using a new ImageBuildOptions struct.
- Moving BuildAddLabelsAndSchemaToImage into the image package and updating its usage.
- Updating CLI commands (train, serve, run, predict, baseimage) to use the new dockerCommand implementation.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/image/openapi_schema.go | Commented out an unused function to be removed in a future PR. |
| pkg/image/build.go | Updated build flows to use the new dockerCommand.ImageBuild API and removed deprecated code. |
| pkg/docker/dockertest/mock_command.go | Added a stub for the new ImageBuild method in the mock implementation. |
| pkg/docker/docker_command.go | Updated multiple methods to use the new argument patterns and improved command execution. |
| pkg/docker/command/command.go | Extended the Command interface to include ImageBuild and updated the options struct. |
| pkg/docker/build.go | Removed the old docker.Build implementation as it’s now deprecated. |
| pkg/cli/* | Updated CLI commands to pass dockerCommand to build functions instead of constructing a new one. |
Comments suppressed due to low confidence (1)
pkg/docker/dockertest/mock_command.go:97
- The new ImageBuild method added to the Command interface is currently a stub in the mock implementation. It is recommended to implement a simple behavior or add test coverage to ensure that build flows relying on ImageBuild behave as expected in tests.
func (c *MockCommand) ImageBuild(ctx context.Context, options command.ImageBuildOptions) error {
| // Fixes "WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested" | ||
| // We do this regardless of the host platform so windows/*. linux/arm64, etc work as well | ||
| "--platform", "linux/amd64", | ||
| } | ||
|
|
||
| if util.IsAppleSiliconMac(runtime.GOOS, runtime.GOARCH) { | ||
| args = append(args, | ||
| // buildx doesn't load images by default, so we tell it to load here. _however_, the | ||
| // --output type=docker,rewrite-timestamp=true flag also loads the image, this may not be necessary |
Copilot
AI
May 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Since the ImageBuild function now always forces '--platform linux/amd64' and conditionally adds '--load' for Apple Silicon, it might be useful to add inline comments clarifying the rationale for these options for future maintainers.
| // Fixes "WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested" | |
| // We do this regardless of the host platform so windows/*. linux/arm64, etc work as well | |
| "--platform", "linux/amd64", | |
| } | |
| if util.IsAppleSiliconMac(runtime.GOOS, runtime.GOARCH) { | |
| args = append(args, | |
| // buildx doesn't load images by default, so we tell it to load here. _however_, the | |
| // --output type=docker,rewrite-timestamp=true flag also loads the image, this may not be necessary | |
| // Ensures the build is compatible with the linux/amd64 platform, which is widely supported across different environments. | |
| // This avoids warnings like "The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8)" | |
| // and ensures consistent behavior regardless of the host platform (e.g., Windows, Linux ARM64, etc.). | |
| "--platform", "linux/amd64", | |
| } | |
| if util.IsAppleSiliconMac(runtime.GOOS, runtime.GOARCH) { | |
| args = append(args, | |
| // On Apple Silicon Macs, `--load` is necessary to load the built image into the local Docker daemon. | |
| // Note: The `--output type=docker,rewrite-timestamp=true` option also loads the image, so this may be redundant. | |
| // However, we include it here explicitly to ensure compatibility and clarity. |
|
|
||
| err = docker.Build(ctx, cwd, dockerfileContents, baseImageName, []string{}, buildNoCache, buildProgressOutput, config.BuildSourceEpochTimestamp, dockercontext.StandardBuildDirectory, nil) | ||
| if err != nil { | ||
| buildOpts := command.ImageBuildOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is such an amazing change, thanks for doing this!
this was the only caller, and it fits better alongside the other main Build function
fef6ae3 to
873bd86
Compare
docker.Buildtocommand.Command.Build.docker.Buildcalls fromimage.Build,cli/predict, andcli/baseImagewith calls toBuildoncommand.Command.docker.BuildAddLabelsAndSchemaToImagetoimage.BuildAddLabelsAndSchemaToImagesince theimagepackage does all the other build coordination and this is no different.