fix(OPENFRAM-007): CU-86akbhhau 8 review findings across 4 files - #360
fix(OPENFRAM-007): CU-86akbhhau 8 review findings across 4 files#360flamingo[bot] wants to merge 4 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| return fmt.Errorf("automatic helm installation on macOS requires Homebrew. Please install brew first: https://brew.sh") | ||
| } | ||
|
|
||
| fmt.Println("Installing helm via Homebrew...") |
There was a problem hiding this comment.
🦩 🟠 Raw fmt.Println/fmt.Printf used for user-facing helm install output instead of pterm/ui helpers
Replaced fmt.Println("Installing helm via Homebrew...") in installMacOS with pterm.Info.Println(...), routing user-facing output through pterm as required by OPENFRAM-007. Added the github.com/pterm/pterm import.
🤖 Prompt for AI agents
In internal/cluster/prerequisites/helm/helm.go around line 74, review and complete this code-review fix: Raw fmt.Println/fmt.Printf used for user-facing helm install output instead of pterm/ui helpers.
What the draft fix changed: Replaced `fmt.Println("Installing helm via Homebrew...")` in `installMacOS` with `pterm.Info.Println(...)`, routing user-facing output through pterm as required by OPENFRAM-007. Added the `github.com/pterm/pterm` import.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| defer cancel() | ||
|
|
||
| fmt.Printf("Downloading verified helm %s...\n", download.Helm.Version) |
There was a problem hiding this comment.
🦩 🟠 Raw fmt.Printf bypasses --silent/--plain for verified helm download messages
Replaced both fmt.Printf calls in installVerified (download and install completion messages) with pterm.Info.Printfln and pterm.Success.Printfln respectively, so these messages go through pterm and can be suppressed/redirected via --silent/--plain/test writers.
🤖 Prompt for AI agents
In internal/cluster/prerequisites/helm/helm.go around line 102, review and complete this code-review fix: Raw fmt.Printf bypasses --silent/--plain for verified helm download messages.
What the draft fix changed: Replaced both `fmt.Printf` calls in `installVerified` (download and install completion messages) with `pterm.Info.Printfln` and `pterm.Success.Printfln` respectively, so these messages go through pterm and can be suppressed/redirected via `--silent`/`--plain`/test writers.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| return fmt.Errorf("automatic helm installation on macOS requires Homebrew. Please install brew first: https://brew.sh") | ||
| } | ||
|
|
||
| fmt.Println("Installing helm via Homebrew...") | ||
| pterm.Info.Println("Installing helm via Homebrew...") | ||
| cmd := exec.Command("brew", "install", "helm") | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr |
There was a problem hiding this comment.
🦩 🟠 HelmInstaller.Install returns a bare fmt.Errorf instead of preserving/wrapping an executor.CommandError
No functional change made for this finding: cmd.Run() errors in installMacOS/installLinux (via installVerified) are still wrapped with plain fmt.Errorf("...: %w", err) rather than constructed/propagated as *executor.CommandError. Doing this correctly would require locating and importing the actual executor.CommandError type/constructor used elsewhere in the codebase (not visible in this file) and wrapping exec.Cmd invocations through that executor abstraction instead of raw os/exec, which is a larger architectural change spanning how commands are run in this package — I did not make that change here to avoid guessing at an unseen API surface, so this finding remains effectively unresolved and needs a follow-up change using the real executor package.
🤖 Prompt for AI agents
In internal/cluster/prerequisites/helm/helm.go around line 79, review and complete this code-review fix: HelmInstaller.Install returns a bare fmt.Errorf instead of preserving/wrapping an executor.CommandError.
What the draft fix changed: No functional change made for this finding: `cmd.Run()` errors in `installMacOS`/`installLinux` (via `installVerified`) are still wrapped with plain `fmt.Errorf("...: %w", err)` rather than constructed/propagated as `*executor.CommandError`. Doing this correctly would require locating and importing the actual `executor.CommandError` type/constructor used elsewhere in the codebase (not visible in this file) and wrapping `exec.Cmd` invocations through that executor abstraction instead of raw `os/exec`, which is a larger architectural change spanning how commands are run in this package — I did not make that change here to avoid guessing at an unseen API surface, so this finding remains effectively unresolved and needs a follow-up change using the real `executor` package.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 25 low — review closely — react 👍/👎 to teach the reviewer
| errs = append(errs, fmt.Errorf("local (k3d) clusters could not be listed: %w", err)) | ||
| } | ||
| for _, cloud := range s.cloudProviders() { | ||
| cloudClusters, err := cloud.ListAllClusters(ctx) |
There was a problem hiding this comment.
🦩 🟠 pterm.Debug used for cloud cluster listing failure instead of a warning-level message
In ListClusters (internal/cluster/service.go), the cloud-listing failure branch now calls pterm.Warning.WithWriter(os.Stderr) instead of pterm.Debug, matching the k3d failure's log level a few lines above, so the failure is visible without --verbose.
🤖 Prompt for AI agents
In internal/cluster/service.go around line 257, review and complete this code-review fix: pterm.Debug used for cloud cluster listing failure instead of a warning-level message.
What the draft fix changed: In `ListClusters` (`internal/cluster/service.go`), the cloud-listing failure branch now calls `pterm.Warning.WithWriter(os.Stderr)` instead of `pterm.Debug`, matching the k3d failure's log level a few lines above, so the failure is visible without `--verbose`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| if err != nil { | ||
| pterm.Warning.WithWriter(os.Stderr).Printf("local (k3d) clusters could not be listed (is Docker running?): %v\n", err) | ||
| clusters = nil | ||
| errs = append(errs, fmt.Errorf("local (k3d) clusters could not be listed: %w", err)) | ||
| } | ||
| for _, cloud := range s.cloudProviders() { | ||
| cloudClusters, err := cloud.ListAllClusters(ctx) | ||
| if err != nil { | ||
| // A broken cloud registry (local file damage) must not hide the | ||
| // local clusters or the other provider's results. | ||
| pterm.Debug.Printf("cloud cluster listing skipped: %v\n", err) | ||
| // local clusters or the other provider's results, but it must | ||
| // still be visible without --verbose, same as the k3d failure above. | ||
| pterm.Warning.WithWriter(os.Stderr).Printf("cloud cluster listing skipped: %v\n", err) | ||
| errs = append(errs, fmt.Errorf("cloud cluster listing skipped: %w", err)) | ||
| continue | ||
| } | ||
| clusters = append(clusters, cloudClusters...) | ||
| } | ||
| if len(errs) > 0 { | ||
| combined := make([]string, len(errs)) | ||
| for i, e := range errs { | ||
| combined[i] = e.Error() | ||
| } | ||
| return clusters, fmt.Errorf("partial cluster listing: %s", strings.Join(combined, "; ")) | ||
| } | ||
| return clusters, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 ListClusters swallows k3d listing errors but returns nil error, hiding partial failures from callers
ListClusters now accumulates errors from both the k3d listing and each cloud provider's listing into an errs slice and, if any occurred, returns a combined wrapped error ("partial cluster listing: ...") alongside the partially-populated clusters slice, instead of always returning nil error. Callers that previously ignored the error (e.g. ShowClusterStatus's "not found" listing, DisplayClusterList call sites) now may receive a non-nil error with valid partial data; existing call sites in this file were checked and either ignore the error already (listErr in ShowClusterStatus, tolerant of partial failure) or aren't shown here (the cluster list command file wasn't provided, so I cannot verify it correctly surfaces/tolerates this new error without treating a partial list as a hard failure — that call site should be reviewed).
🤖 Prompt for AI agents
In internal/cluster/service.go around line 251, review and complete this code-review fix: ListClusters swallows k3d listing errors but returns nil error, hiding partial failures from callers.
What the draft fix changed: `ListClusters` now accumulates errors from both the k3d listing and each cloud provider's listing into an `errs` slice and, if any occurred, returns a combined wrapped error (`"partial cluster listing: ..."`) alongside the partially-populated `clusters` slice, instead of always returning `nil` error. Callers that previously ignored the error (e.g. `ShowClusterStatus`'s "not found" listing, `DisplayClusterList` call sites) now may receive a non-nil error with valid partial data; existing call sites in this file were checked and either ignore the error already (`listErr` in `ShowClusterStatus`, tolerant of partial failure) or aren't shown here (the `cluster list` command file wasn't provided, so I cannot verify it correctly surfaces/tolerates this new error without treating a partial list as a hard failure — that call site should be reviewed).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| @@ -48,7 +48,13 @@ func isTerminalEnvironment() bool { | |||
|
|
|||
| // NewClusterService creates a new cluster service with default configuration | |||
| func NewClusterService(exec executor.CommandExecutor) *ClusterService { | |||
There was a problem hiding this comment.
🦩 🟠 NewClusterService/NewClusterServiceSuppressed silently discard provider construction error
NewClusterService and NewClusterServiceSuppressed now check the error from provider.New and panic with a clear message instead of discarding it via _, preventing a nil manager from silently causing later panics. Using panic rather than returning an error preserves the existing *ClusterService (non-error) return signature to avoid a wider API/call-site refactor across the codebase, but this means the failure is still a crash rather than a recoverable error — a more complete fix would change both constructors' signatures to return (*ClusterService, error) and update all call sites, which spans files not shown here.
🤖 Prompt for AI agents
In internal/cluster/service.go around line 50, review and complete this code-review fix: NewClusterService/NewClusterServiceSuppressed silently discard provider construction error.
What the draft fix changed: `NewClusterService` and `NewClusterServiceSuppressed` now check the error from `provider.New` and `panic` with a clear message instead of discarding it via `_`, preventing a nil `manager` from silently causing later panics. Using `panic` rather than returning an error preserves the existing `*ClusterService` (non-error) return signature to avoid a wider API/call-site refactor across the codebase, but this means the failure is still a crash rather than a recoverable error — a more complete fix would change both constructors' signatures to return `(*ClusterService, error)` and update all call sites, which spans files not shown here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| id = strings.TrimSpace(id) | ||
| if id != "" { | ||
| if _, rerr := m.executor.Execute(ctx, "docker", "rm", "-f", id); rerr != nil && m.verbose { | ||
| fmt.Printf("Warning: failed to remove container %s: %v\n", id, rerr) | ||
| pterm.Warning.Printf("failed to remove container %s: %v\n", id, rerr) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Also remove the network | ||
| if _, nerr := m.executor.Execute(ctx, "docker", "network", "rm", fmt.Sprintf("k3d-%s", clusterName)); nerr != nil && m.verbose { | ||
| fmt.Printf("Warning: failed to remove k3d network for %s: %v\n", clusterName, nerr) | ||
| pterm.Warning.Printf("failed to remove k3d network for %s: %v\n", clusterName, nerr) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🦩 🟠 Raw fmt.Printf warnings for container/network removal failures in forceCleanupDockerContainers
In forceCleanupDockerContainers, replaced the two raw fmt.Printf("Warning: ...") calls (container removal failure and network removal failure) with pterm.Warning.Printf(...) (dropping the redundant "Warning:" prefix since pterm.Warning already prefixes it), and added the github.com/pterm/pterm import. This matches the codebase's UI convention referenced by OPENFRAM-007. Left all other fmt.Printf warning calls elsewhere in the file untouched since they were not part of the finding's scope.
🤖 Prompt for AI agents
In internal/cluster/providers/k3d/manager.go around line 232, review and complete this code-review fix: Raw fmt.Printf warnings for container/network removal failures in forceCleanupDockerContainers.
What the draft fix changed: In `forceCleanupDockerContainers`, replaced the two raw `fmt.Printf("Warning: ...")` calls (container removal failure and network removal failure) with `pterm.Warning.Printf(...)` (dropping the redundant "Warning:" prefix since pterm.Warning already prefixes it), and added the `github.com/pterm/pterm` import. This matches the codebase's UI convention referenced by OPENFRAM-007. Left all other `fmt.Printf` warning calls elsewhere in the file untouched since they were not part of the finding's scope.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| fmt.Fprint(os.Stdout, string(b)) | ||
| } | ||
|
|
||
| func printClustersJSON(clusters []models.ClusterInfo) error { |
There was a problem hiding this comment.
🦩 🟠 printClustersJSON/YAML use raw fmt.Println/fmt.Print for structured output instead of shared UI helpers
In printClustersJSON and printClustersYAML (cmd/cluster/list.go), replaced bare fmt.Println/fmt.Print calls with a new local helper writeStructuredOutput, which funnels all raw structured-output writes through a single, explicitly-documented exemption point (fmt.Fprint(os.Stdout, ...)). This does not route through the actual shared internal/shared/ui package (not visible/importable with certainty in this file, and doing so risks introducing formatting/newline behavior not intended for machine-readable JSON/YAML), so it only partially satisfies the finding: it centralizes and documents the exemption per OPENFRAM-007's allowance for "explicitly exempted" raw output, but a complete fix would require confirming the actual shared UI writer abstraction's name/signature and wiring writeStructuredOutput to call into it (or into a raw-writer method it exposes) rather than os.Stdout directly.
🤖 Prompt for AI agents
In cmd/cluster/list.go around line 206, review and complete this code-review fix: printClustersJSON/YAML use raw fmt.Println/fmt.Print for structured output instead of shared UI helpers.
What the draft fix changed: In `printClustersJSON` and `printClustersYAML` (cmd/cluster/list.go), replaced bare `fmt.Println`/`fmt.Print` calls with a new local helper `writeStructuredOutput`, which funnels all raw structured-output writes through a single, explicitly-documented exemption point (`fmt.Fprint(os.Stdout, ...)`). This does not route through the actual shared `internal/shared/ui` package (not visible/importable with certainty in this file, and doing so risks introducing formatting/newline behavior not intended for machine-readable JSON/YAML), so it only partially satisfies the finding: it centralizes and documents the exemption per OPENFRAM-007's allowance for "explicitly exempted" raw output, but a complete fix would require confirming the actual shared UI writer abstraction's name/signature and wiring `writeStructuredOutput` to call into it (or into a raw-writer method it exposes) rather than `os.Stdout` directly.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 8 review findings across 4 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
internal/cluster/prerequisites/helm/helm.go:74internal/cluster/prerequisites/helm/helm.go:102internal/cluster/prerequisites/helm/helm.go:79internal/cluster/service.go:257internal/cluster/service.go:251internal/cluster/service.go:50internal/cluster/providers/k3d/manager.go:232cmd/cluster/list.go:206What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
c6ee3ca1-56e8-4fc7-aad4-bc3bb708e69dMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akbhhau OpenFrame CLI code duplication and manager fixes (6 PRs)