feat: support Apple container#614
Conversation
|
| Branch | Total Count |
|---|---|
| main | 5346 |
| This PR | 5691 |
| Difference | +345 (6.45%) |
📁 Changes by file type:
| File Type | Change |
|---|---|
| Go files (.go) | ❌ +1 |
| Documentation (.md) | ➖ No change |
| Earthfiles | ➖ No change |
Keep up the great work migrating from Earthly to Earthbuild! 🚀
💡 Tips for finding more occurrences
Run locally to see detailed breakdown:
./.github/scripts/count-earthly.shNote that the goal is not to reach 0.
There is anticipated to be at least some occurences of earthly in the source code due to backwards compatibility with config files and language constructs.
There was a problem hiding this comment.
Code Review
This pull request introduces support for a new container frontend, "Apple Container" (using the container binary), alongside Docker and Podman. It adds the implementation for managing containers, images, and volumes under this frontend, updates configuration and autodetection logic, and includes corresponding unit tests. A review comment points out critical resource leak and error-handling issues in the ImageLoad function of the new frontend, suggesting an anonymous function wrapper to properly scope deferred file closures and cleanups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (asf *appleShellFrontend) ImageLoad(ctx context.Context, images ...io.Reader) error { | ||
| var err error | ||
|
|
||
| for _, image := range images { | ||
| file, tmpErr := os.CreateTemp("", "earthly-apple-load-*") | ||
| if tmpErr != nil { | ||
| err = multierror.Append(err, errors.Wrap(tmpErr, "failed to create temp tarball")) | ||
| continue | ||
| } | ||
|
|
||
| _, copyErr := io.Copy(file, image) | ||
| if copyErr != nil { | ||
| err = multierror.Append(err, errors.Wrapf(tmpErr, "failed to write to %s", file.Name())) | ||
| continue | ||
| } | ||
| defer file.Close() | ||
| defer os.Remove(file.Name()) | ||
|
|
||
| output, cmdErr := asf.commandContextOutput(ctx, "image", "load", "--input", file.Name()) | ||
| if cmdErr != nil { | ||
| err = multierror.Append(err, errors.Wrapf(cmdErr, "image load failed: %s", output.string())) | ||
| } | ||
| } | ||
|
|
||
| return err | ||
| } |
There was a problem hiding this comment.
There are three distinct issues in this function:
- Resource Leak in Loop:
defer file.Close()anddefer os.Remove(...)are used inside aforloop. In Go,deferis evaluated when the surrounding function returns, not when the loop iteration ends. This will keep all temporary files open and on disk until the entireImageLoadfunction completes. - Permanent Leak on Copy Error: If
io.Copyfails, the loopcontinues before thedeferstatements are registered, meaning the temporary file is never closed or deleted. - Wrapping
nilError: WhencopyErr != nil, the code wrapstmpErr(which isnilat this point) instead ofcopyErr. This silences the actual copy error.
Wrapping the loop body in an anonymous function resolves the defer scope issue, ensures files are cleaned up even on copy errors, and allows us to correctly wrap copyErr.
func (asf *appleShellFrontend) ImageLoad(ctx context.Context, images ...io.Reader) error {
var err error
for _, image := range images {
func() {
file, tmpErr := os.CreateTemp("", "earthly-apple-load-*")
if tmpErr != nil {
err = multierror.Append(err, errors.Wrap(tmpErr, "failed to create temp tarball"))
return
}
defer file.Close()
defer os.Remove(file.Name())
_, copyErr := io.Copy(file, image)
if copyErr != nil {
err = multierror.Append(err, errors.Wrapf(copyErr, "failed to write to %s", file.Name()))
return
}
output, cmdErr := asf.commandContextOutput(ctx, "image", "load", "--input", file.Name())
if cmdErr != nil {
err = multierror.Append(err, errors.Wrapf(cmdErr, "image load failed: %s", output.string()))
}
}()
}
return err
}References
- In Go, avoid using an if statement with an initializer. Instead, declare the variable on a preceding line to prevent potential conflicts with project linting rules.
|
Blocked by #615 |
No description provided.