feat: restore harness-config build UI and executor#420
Conversation
Restores the build feature code that was accidentally removed by PR GoogleCloudPlatform#412. This includes: - BuildHarnessConfigImageExecutor in maintenance_executors.go - build-harness-config-image seeded operation - Executor wiring in admin_maintenance.go - Build Image button, dialog, and log streaming UI on harness-config detail page Originally shipped in PRs GoogleCloudPlatform#406 and GoogleCloudPlatform#410.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new maintenance operation, build-harness-config-image, which builds and optionally pushes a container image from a harness-config's bundled Dockerfile, along with a corresponding UI in the harness-config detail page to trigger and monitor the build. The feedback highlights several critical improvements: wrapping the file-writing loop in an anonymous function to leverage defer for safer resource cleanup, sanitizing the derived image name to prevent Docker build failures from invalid characters, failing fast when a push is requested without a configured registry, and checking the component's connection status in the UI to prevent memory and network leaks during polling.
| for _, f := range hc.Files { | ||
| objectPath := hc.StoragePath + "/" + f.Path | ||
| reader, _, err := e.storage.Download(ctx, objectPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to download %q from storage: %w", f.Path, err) | ||
| } | ||
|
|
||
| destPath := filepath.Join(tmpDir, f.Path) | ||
| if !strings.HasPrefix(destPath, tmpDir+string(os.PathSeparator)) { | ||
| _ = reader.Close() | ||
| return fmt.Errorf("invalid file path %q: escapes build directory", f.Path) | ||
| } | ||
| if dir := filepath.Dir(destPath); dir != tmpDir { | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { | ||
| _ = reader.Close() | ||
| return fmt.Errorf("failed to create directory for %q: %w", f.Path, err) | ||
| } | ||
| } | ||
|
|
||
| outFile, err := os.Create(destPath) | ||
| if err != nil { | ||
| _ = reader.Close() | ||
| return fmt.Errorf("failed to create file %q: %w", f.Path, err) | ||
| } | ||
| _, err = io.Copy(outFile, reader) | ||
| _ = reader.Close() | ||
| _ = outFile.Close() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to write file %q: %w", f.Path, err) | ||
| } | ||
|
|
||
| if f.Mode != "" { | ||
| mode := os.FileMode(0o644) | ||
| if _, err := fmt.Sscanf(f.Mode, "%o", &mode); err == nil { | ||
| _ = os.Chmod(destPath, mode) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrapping the loop body in an anonymous function allows using defer for clean and robust resource management. This avoids having to manually call Close() on the reader and writer on every error path, reducing the risk of resource leaks.
for _, f := range hc.Files {
if err := func() error {
objectPath := hc.StoragePath + "/" + f.Path
reader, _, err := e.storage.Download(ctx, objectPath)
if err != nil {
return fmt.Errorf("failed to download %q from storage: %w", f.Path, err)
}
defer reader.Close()
destPath := filepath.Join(tmpDir, f.Path)
if !strings.HasPrefix(destPath, tmpDir+string(os.PathSeparator)) {
return fmt.Errorf("invalid file path %q: escapes build directory", f.Path)
}
if dir := filepath.Dir(destPath); dir != tmpDir {
if err := os.MkdirAll(dir, 0o755); err != nil {
return fmt.Errorf("failed to create directory for %q: %w", f.Path, err)
}
}
outFile, err := os.Create(destPath)
if err != nil {
return fmt.Errorf("failed to create file %q: %w", f.Path, err)
}
defer outFile.Close()
if _, err = io.Copy(outFile, reader); err != nil {
return fmt.Errorf("failed to write file %q: %w", f.Path, err)
}
if f.Mode != "" {
mode := os.FileMode(0o644)
if _, err := fmt.Sscanf(f.Mode, "%o", &mode); err == nil {
_ = os.Chmod(destPath, mode)
}
}
return nil
}(); err != nil {
return err
}
}| imageName := hc.Slug | ||
| if imageName == "" { | ||
| imageName = hc.Name | ||
| } |
There was a problem hiding this comment.
If hc.Slug is empty, imageName defaults to hc.Name. However, hc.Name can contain uppercase letters, spaces, or other characters that are invalid for OCI/Docker repository names. This will cause the docker build command to fail with an invalid reference format error. We should sanitize the image name to ensure it is lowercase and contains only valid characters.
imageName := hc.Slug
if imageName == "" {
imageName = hc.Name
}
imageName = strings.ToLower(imageName)
var sanitized []rune
for _, r := range imageName {
if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '_' || r == '-' || r == '.' {
sanitized = append(sanitized, r)
} else if r == ' ' {
sanitized = append(sanitized, '-')
}
}
imageName = string(sanitized)
imageName = strings.Trim(imageName, "-.")
if imageName == "" {
return fmt.Errorf("invalid image name derived from harness-config name %q", hc.Name)
}| if params["push"] == "true" && registry != "" { | ||
| pushImage := registry + "/" + outputImage |
There was a problem hiding this comment.
If the user explicitly requests a push (push == "true") but no registry is configured or provided, the push is silently skipped without informing the user. It is better to fail fast and return an error so the user knows the push could not be completed.
| if params["push"] == "true" && registry != "" { | |
| pushImage := registry + "/" + outputImage | |
| if params["push"] == "true" { | |
| if registry == "" { | |
| return fmt.Errorf("push requested but no image registry is configured") | |
| } | |
| pushImage := registry + "/" + outputImage |
| private async pollBuildStatus(): Promise<void> { | ||
| if (!this.buildRunId) return; | ||
|
|
||
| try { | ||
| const resp = await apiFetch( | ||
| `/api/v1/admin/maintenance/operations/build-harness-config-image/runs/${this.buildRunId}`, | ||
| ); |
There was a problem hiding this comment.
If the component is disconnected from the DOM while the apiFetch is in-flight, this.buildPollTimer is null (since it hasn't been scheduled yet). When the fetch resolves, this.buildRunning is still true, so a new timeout is scheduled. This causes a memory and network leak because the polling loop continues indefinitely in the background on a destroyed component. We should check this.isConnected before continuing the polling loop.
private async pollBuildStatus(): Promise<void> {
if (!this.buildRunId || !this.isConnected) return;
try {
const resp = await apiFetch(
`/api/v1/admin/maintenance/operations/build-harness-config-image/runs/${this.buildRunId}`,
);
if (!this.isConnected) return;
Restores the build feature code that was accidentally removed by PR #412. This includes:
Originally shipped in PRs #406 and #410.
Fixes #<issue_number_goes_here>