refactor(devcontainer/buildkit): reduce cyclomatic complexity#383
refactor(devcontainer/buildkit): reduce cyclomatic complexity#383
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
📝 WalkthroughWalkthroughReplaces positional BuildRemote parameters with a BuildRemoteOptions struct, reorganizes remote build into validated phases (validation, BuildKit client setup, image resolution, existing-image short-circuit, registry auth, build-option preparation, local mounts, solve execution, image detail retrieval), and converts NewOptions to accept NewOptionsParams with updated callsites. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Remote as BuildRemote
participant BK as BuildKitClient
participant Reg as Registry
participant FS as LocalFS
Caller->>Remote: BuildRemote(ctx, BuildRemoteOptions)
Remote->>Remote: validateRemoteBuildOptions(opts)
Remote->>BK: setupBuildKitClient(ctx, opts.Options)
Remote->>Reg: resolveImageReference(imageName)
Remote->>Reg: setupRegistryAuth(ref, keychain)
Remote->>Reg: checkExistingImage(ref, opts)
alt existing image found
Remote-->>Caller: return existing BuildInfo
else
Remote->>Remote: prepareBuildOptions(...)
Remote->>FS: setupLocalMounts(...)
Remote->>BK: executeBuild(solveOpt, sessions)
BK->>Reg: push/check push permission (if needed)
Remote->>Reg: getImageDetails(ref, opts.TargetArch, keychain)
Remote-->>Caller: return new BuildInfo
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/devcontainer/buildkit/remote.go`:
- Around line 163-193: The temp cert directory returned by ensureCertPaths
(tmpDir) must be removed on error paths to avoid leaking sensitive files; in
setupBuildKitClient, after calling ensureCertPaths ensure you call
os.RemoveAll(tmpDir) before returning when client.New fails and also after
c.Close() when c.Info(timeoutCtx) fails (or if any subsequent error occurs), but
do not remove tmpDir on the successful return path; reference the tmpDir
variable, ensureCertPaths call, client.New call, and the c.Close()/c.Info error
branch to locate where to add the cleanup.
- Around line 144-159: validateRemoteBuildOptions currently allows
options.SkipPush with an empty CLIOptions.Platform.Build.Repository but the
remote flow assumes a registry push; update validateRemoteBuildOptions to reject
SkipPush for remote builds by returning an error when options.SkipPush is true
(in the same function that checks CLIOptions.Platform.Build and
Build.Repository) so remote builds always have a repository and push enabled,
referencing validateRemoteBuildOptions, options.SkipPush,
CLIOptions.Platform.Build and Build.Repository to locate the change.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/devcontainer/buildkit/remote.go (1)
432-437: ValidatetargetArchbefore using it inremote.WithPlatform.According to go-containerregistry documentation, passing an empty
Architecturetoremote.WithPlatformcreates a wildcard that matches anylinux/*manifest non-deterministically, potentially selecting the wrong image. While your driver implementations validate and return architecture values, adding a guard ensures robustness against edge cases.🛠️ Suggested guard
- remoteImage, err := remote.Image(ref, - remote.WithAuthFromKeychain(keychain), - remote.WithPlatform(v1.Platform{Architecture: targetArch, OS: "linux"}), - ) + remoteOpts := []remote.Option{ + remote.WithAuthFromKeychain(keychain), + } + if targetArch != "" { + remoteOpts = append(remoteOpts, remote.WithPlatform(v1.Platform{Architecture: targetArch, OS: "linux"})) + } + remoteImage, err := remote.Image(ref, remoteOpts...)
🤖 Fix all issues with AI agents
In `@pkg/devcontainer/buildkit/remote.go`:
- Around line 59-60: The repository string may contain a trailing slash which
leads to a double-slash in imageName; before building imageName (where imageName
:= opts.Options.CLIOptions.Platform.Build.Repository + "/" +
build.GetImageName(...)), normalize the repository by trimming any trailing
slash (e.g., with strings.TrimRight or strings.TrimSuffix) and then concatenate
with build.GetImageName so resolveImageReference(ctx, imageName) receives a
valid reference.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/devcontainer/buildkit/remote.go (1)
460-500: Critical bug:ensureCertPathsreturnscaPathinstead ofcertPath.All return statements return
caPathas the 4th value instead ofcertPath:return parentDir, caPath, keyPath, caPath, ... // Should be certPathThis causes
setupBuildKitClientto receive the CA path where it expects the certificate path, breaking mTLS authentication with the BuildKit server.🐛 Proposed fix
func ensureCertPaths(buildOpts *devpod.PlatformBuildOptions) (parentDir string, caPath string, keyPath string, certPath string, err error) { parentDir, err = os.MkdirTemp("", "build-certs-*") if err != nil { - return parentDir, caPath, keyPath, caPath, fmt.Errorf("create temp dir %w", err) + return parentDir, caPath, keyPath, certPath, fmt.Errorf("create temp dir %w", err) } // write CA caPath = filepath.Join(parentDir, "ca.pem") caBytes, err := base64.StdEncoding.DecodeString(buildOpts.CertCA) if err != nil { - return parentDir, caPath, keyPath, caPath, fmt.Errorf("decode CA %w", err) + return parentDir, caPath, keyPath, certPath, fmt.Errorf("decode CA %w", err) } err = os.WriteFile(caPath, caBytes, 0o700) if err != nil { - return parentDir, caPath, keyPath, caPath, fmt.Errorf("write CA file %w", err) + return parentDir, caPath, keyPath, certPath, fmt.Errorf("write CA file %w", err) } // write key keyPath = filepath.Join(parentDir, "key.pem") keyBytes, err := base64.StdEncoding.DecodeString(buildOpts.CertKey) if err != nil { - return parentDir, caPath, keyPath, caPath, fmt.Errorf("decode private key %w", err) + return parentDir, caPath, keyPath, certPath, fmt.Errorf("decode private key %w", err) } err = os.WriteFile(keyPath, keyBytes, 0o700) if err != nil { - return parentDir, caPath, keyPath, caPath, fmt.Errorf("write private key file %w", err) + return parentDir, caPath, keyPath, certPath, fmt.Errorf("write private key file %w", err) } // write cert certPath = filepath.Join(parentDir, "cert.pem") certBytes, err := base64.StdEncoding.DecodeString(buildOpts.Cert) if err != nil { - return parentDir, caPath, keyPath, caPath, fmt.Errorf("decode cert %w", err) + return parentDir, caPath, keyPath, certPath, fmt.Errorf("decode cert %w", err) } err = os.WriteFile(certPath, certBytes, 0o700) if err != nil { - return parentDir, caPath, keyPath, caPath, fmt.Errorf("write cert file %w", err) + return parentDir, caPath, keyPath, certPath, fmt.Errorf("write cert file %w", err) } - return parentDir, caPath, keyPath, caPath, nil + return parentDir, caPath, keyPath, certPath, nil }
🤖 Fix all issues with AI agents
In `@pkg/devcontainer/buildkit/remote.go`:
- Around line 158-166: Remove the redundant repository check: in the block that
validates options (referencing options.CLIOptions.Platform.Build.Repository and
options.SkipPush), delete the duplicate "if
options.CLIOptions.Platform.Build.Repository == \"\" { return
errors.New(\"remote builds require a registry to be provided\") }" that appears
after the SkipPush check; keep the existing two checks—one that errors when
Repository=="" && !SkipPush, and the one that errors when options.SkipPush is
true—so the validation logic remains correct and no unreachable code remains.
Signed-off-by: GitHub <noreply@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/devcontainer/buildkit/remote.go`:
- Around line 489-492: Change the file permissions used when writing
certificate/key files from 0o700 to 0o600 to remove unnecessary execute bits;
locate the os.WriteFile call that writes certBytes to filePath (the block that
returns fmt.Errorf("write %s file %w", certType, err)) and update the permission
argument to 0o600 so the files are created with owner read/write only.
🧹 Nitpick comments (1)
pkg/devcontainer/buildkit/remote.go (1)
227-241: Consider logging the error when image check fails.The function silently discards all errors from
getImageDetails, proceeding to build regardless of the failure reason. While this fault-tolerant approach is reasonable, logging the error at debug level would aid troubleshooting when unexpected rebuilds occur due to transient network issues, auth failures, or rate limits.♻️ Suggested improvement
func checkExistingImage(opts checkExistingImageOptions) (*config.BuildInfo, bool) { imageDetails, err := getImageDetails(opts.Ref, opts.TargetArch, opts.Keychain) if err == nil { opts.Log.Infof("skipping build because an existing image was found %s", opts.ImageName) return &config.BuildInfo{ ImageDetails: imageDetails, ImageMetadata: opts.ExtendedBuildInfo.MetadataConfig, ImageName: opts.ImageName, PrebuildHash: opts.PrebuildHash, RegistryCache: opts.Options.RegistryCache, Tags: opts.Options.Tag, }, true } + opts.Log.Debugf("existing image check failed, proceeding with build: %v", err) return nil, false }
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/devcontainer/build/options.go`:
- Around line 83-88: The code in NewOptions appends image tags using
params.Options.Repository, params.Options.PrebuildRepositories and
params.PrebuildHash which can produce invalid tags like "repo:" when
PrebuildHash is empty; fix this by normalizing params.PrebuildHash to "latest"
(or use that fallback value) before any concatenation in NewOptions so that when
building buildOptions.Images you always append "<repo>:<hash>" with a non-empty
hash; update the logic that references params.PrebuildHash in NewOptions to use
the normalized value.
In `@pkg/devcontainer/buildkit/remote.go`:
- Around line 188-204: The function checkExistingImage accesses
params.Opts.ExtendedBuildInfo.MetadataConfig without guarding for nil (same
issue as in BuildRemote); update checkExistingImage to check if params.Opts !=
nil and params.Opts.ExtendedBuildInfo != nil before reading MetadataConfig and
only set ImageMetadata when ExtendedBuildInfo is non-nil (otherwise leave
ImageMetadata nil or a safe default), ensuring no nil dereference when returning
the config.BuildInfo.
- Around line 47-108: The code in BuildRemote reads
opts.ExtendedBuildInfo.MetadataConfig without ensuring ExtendedBuildInfo is
non-nil; update BuildRemote so before constructing the config.BuildInfo it
checks if opts.ExtendedBuildInfo != nil and uses
opts.ExtendedBuildInfo.MetadataConfig when present, otherwise pass a nil/zero
value (e.g., nil or empty struct) for ImageMetadata; adjust the return value
construction (config.BuildInfo) to reference this local metadata variable to
avoid a nil dereference panic.
🧹 Nitpick comments (3)
pkg/devcontainer/build/options.go (1)
50-71: Consider adding a nil check forParsedConfig.Lines 61, 64, and 89 access
params.ParsedConfigwithout nil checks. While callers may guarantee non-nil values, defensive validation at the function entry would prevent potential nil pointer dereferences.🛡️ Optional defensive check
func NewOptions(params NewOptionsParams) (*BuildOptions, error) { + if params.ParsedConfig == nil { + return nil, fmt.Errorf("ParsedConfig is required") + } var err errorpkg/devcontainer/buildkit/remote.go (2)
378-403: Consider passing context togetImageDetailsfor cancellation support.The
ctxparameter was removed fromgetImageDetails, butremote.Imagesupports context viaremote.WithContext(ctx). Without it, long-running registry calls cannot be cancelled.♻️ Suggested change
-func getImageDetails(ref name.Reference, targetArch string, keychain authn.Keychain) (*config.ImageDetails, error) { +func getImageDetails(ctx context.Context, ref name.Reference, targetArch string, keychain authn.Keychain) (*config.ImageDetails, error) { remoteImage, err := remote.Image(ref, remote.WithAuthFromKeychain(keychain), remote.WithPlatform(v1.Platform{Architecture: targetArch, OS: "linux"}), + remote.WithContext(ctx), )Update call sites accordingly:
- imageDetails, err := getImageDetails(params.Ref, params.TargetArch, params.Keychain) + imageDetails, err := getImageDetails(ctx, params.Ref, params.TargetArch, params.Keychain)
236-328:prepareSolveOptionsis still fairly long but well-organized.While this function handles many concerns, the logic is linear and each section is clearly commented. Consider extracting the exports configuration (lines 307-317) and frontend attributes setup (lines 319-325) into separate helpers if this grows further.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/devcontainer/buildkit/remote.go`:
- Around line 59-60: The code builds a container image reference using
filepath.Join which emits OS-specific separators; change the join to use
path.Join (from the "path" package) instead of filepath.Join when constructing
imageName (the line using filepath.Join(repo,
build.GetImageName(opts.LocalWorkspaceFolder, opts.PrebuildHash))). Update
imports to include "path" and remove or keep "filepath" only if still needed
elsewhere so the image reference always uses forward slashes for registry paths.
🧹 Nitpick comments (1)
pkg/devcontainer/buildkit/remote.go (1)
402-415: Consider handling empty exports defensively.Line 403 accesses
params.SolveOpts.Exports[0]without checking if the slice is non-empty. While the current code flow guaranteesaddExportsis called beforeexecuteBuild, a defensive check would prevent potential panics if the code is refactored later.♻️ Optional defensive check
func executeBuild(params executeBuildParams) error { + imageName := "unknown" + if len(params.SolveOpts.Exports) > 0 { + imageName = params.SolveOpts.Exports[0].Attrs[string(exptypes.OptKeyName)] + } - params.Logger.Infof("start building %s using platform builder (%s)", params.SolveOpts.Exports[0].Attrs[string(exptypes.OptKeyName)], params.Info.BuildkitVersion.Version) + params.Logger.Infof("start building %s using platform builder (%s)", imageName, params.Info.BuildkitVersion.Version)
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/devcontainer/buildkit/remote.go`:
- Around line 453-480: ensureCertPaths creates a temp directory (parentDir) and
then calls writeCertFile multiple times but never removes parentDir if any
writeCertFile call fails, leaking files; update ensureCertPaths so that on any
error returned from writeCertFile (for CA, key, or cert) it removes the created
temp directory (e.g., call os.RemoveAll(parentDir) before returning the error)
or use a deferred cleanup that only runs on error, ensuring that the parentDir
is preserved on successful return of certPaths.
🧹 Nitpick comments (2)
pkg/devcontainer/buildkit/remote.go (2)
250-254: Variablesessionshadows the imported package name.The variable
sessionon line 251 shadows thesessionpackage imported on line 24. Consider renaming toauthSessionorsessionAttachablesfor clarity.♻️ Suggested rename
func prepareSolveOptions(ref name.Reference, keychain authn.Keychain, imageName string, opts BuildRemoteOptions) (client.SolveOpt, error) { - session, err := setupRegistryAuth(ref, keychain) + authSession, err := setupRegistryAuth(ref, keychain) if err != nil { return client.SolveOpt{}, err }And update line 286:
- Session: session, + Session: authSession,
403-405: Consider defensive bounds check forExportsaccess.Line 404 accesses
params.SolveOpts.Exports[0]without verifying the slice is non-empty. While the current code path guaranteesaddExportspopulates this, a defensive check would prevent panics if the code evolves.🛡️ Optional defensive check
func executeBuild(params executeBuildParams) error { + imageName := "unknown" + if len(params.SolveOpts.Exports) > 0 { + imageName = params.SolveOpts.Exports[0].Attrs[string(exptypes.OptKeyName)] + } - params.Logger.Infof("start building %s using platform builder (%s)", params.SolveOpts.Exports[0].Attrs[string(exptypes.OptKeyName)], params.Info.BuildkitVersion.Version) + params.Logger.Infof("start building %s using platform builder (%s)", imageName, params.Info.BuildkitVersion.Version)
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K skevetter@pm.me
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.