fix(OPENFRAM-007): 3 review findings in manager.go - #351
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| fmt.Printf("Warning: Could not increase inotify limits: %v\n", err) | ||
| pterm.Warning.Printf("Could not increase inotify limits: %v\n", err) | ||
| } | ||
| // Don't fail - cluster might still work if limits are already sufficient |
There was a problem hiding this comment.
🦩 🟠 Raw fmt.Printf used for user-facing warnings in K3dManager.CreateCluster/DeleteCluster instead of pterm
In CreateCluster, replaced fmt.Printf("Warning: ...") calls (inotify limits, kubeconfig directory prep, stale lock file cleanup, permission fix, post-permission lock cleanup) with pterm.Warning.Printf(...), dropping the redundant "Warning: " prefix since pterm.Warning already renders a warning-level prefix. Also applied the same substitution to the analogous warnings in forceCleanupDockerContainers and increaseInotifyLimitsFor (container/network removal failures) and converted their verbose success messages (fmt.Printf("✓ ...")/fmt.Println("✓ ...")) to pterm.Success.Printf/pterm.Success.Println for consistency with the pterm-based UI contract; added the github.com/pterm/pterm import.
🤖 Prompt for AI agents
In internal/cluster/providers/k3d/manager.go around line 73, review and complete this code-review fix: Raw fmt.Printf used for user-facing warnings in K3dManager.CreateCluster/DeleteCluster instead of pterm.
What the draft fix changed: In `CreateCluster`, replaced `fmt.Printf("Warning: ...")` calls (inotify limits, kubeconfig directory prep, stale lock file cleanup, permission fix, post-permission lock cleanup) with `pterm.Warning.Printf(...)`, dropping the redundant "Warning: " prefix since `pterm.Warning` already renders a warning-level prefix. Also applied the same substitution to the analogous warnings in `forceCleanupDockerContainers` and `increaseInotifyLimitsFor` (container/network removal failures) and converted their verbose success messages (`fmt.Printf("✓ ...")`/`fmt.Println("✓ ...")`) to `pterm.Success.Printf`/`pterm.Success.Println` for consistency with the pterm-based UI contract; added the `github.com/pterm/pterm` import.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
| // Prepare kubeconfig directory before k3d operations (Windows/WSL and Linux CI) | ||
| if err := m.prepareKubeconfigDirectory(ctx); err != nil { | ||
| if m.verbose { | ||
| fmt.Printf("Warning: Could not prepare kubeconfig directory: %v\n", err) |
There was a problem hiding this comment.
🦩 🟠 fmt.Printf DEBUG dump of config file bypasses pterm/--silent contract
In CreateCluster, the verbose DEBUG dump of the generated k3d config file (fmt.Printf("DEBUG: Config file content for %s:\n%s\n", ...)) was changed to pterm.Debug.Printf("Config file content for %s:\n%s\n", ...), removing the manual "DEBUG: " prefix since pterm.Debug supplies its own level indicator.
🤖 Prompt for AI agents
In internal/cluster/providers/k3d/manager.go around line 92, review and complete this code-review fix: fmt.Printf DEBUG dump of config file bypasses pterm/--silent contract.
What the draft fix changed: In `CreateCluster`, the verbose DEBUG dump of the generated k3d config file (`fmt.Printf("DEBUG: Config file content for %s:\n%s\n", ...)`) was changed to `pterm.Debug.Printf("Config file content for %s:\n%s\n", ...)`, removing the manual "DEBUG: " prefix since `pterm.Debug` supplies its own level indicator.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| // No Windows branch: the CLI forwards into WSL and runs as linux (see wsllauncher). | ||
| if force { | ||
| if m.verbose { | ||
| fmt.Printf("k3d delete failed, attempting direct Docker cleanup for cluster %s: %v\n", name, err) | ||
| pterm.Warning.Printf("k3d delete failed, attempting direct Docker cleanup for cluster %s: %v\n", name, err) | ||
| } | ||
| if cleanupErr := m.forceCleanupDockerContainers(ctx, name); cleanupErr != nil { | ||
| // Return original error if cleanup also fails | ||
| return models.NewClusterOperationError("delete", name, fmt.Errorf("failed to delete cluster %s (cleanup also failed: %w): %w", name, cleanupErr, err)) | ||
| } | ||
| // Cleanup succeeded, cluster is removed | ||
| if m.verbose { | ||
| fmt.Printf("✓ Cluster %s removed via direct Docker cleanup\n", name) | ||
| pterm.Success.Printf("Cluster %s removed via direct Docker cleanup\n", name) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🦩 🟠 fmt.Printf success/warning messages in DeleteCluster fallback path bypass pterm
In DeleteCluster's Docker cleanup fallback path, the pre-cleanup verbose message was converted to pterm.Warning.Printf("k3d delete failed, attempting direct Docker cleanup for cluster %s: %v\n", name, err) and the post-cleanup success message was converted to pterm.Success.Printf("Cluster %s removed via direct Docker cleanup\n", name) (dropped the manual "✓ " prefix since pterm.Success renders its own success indicator). Risk: pterm's default writer target and exact prefix formatting were assumed consistent with other usages in the package (e.g. operations.go); if this package configures a custom pterm writer/silence toggle elsewhere, verify these new calls respect it identically to existing pterm calls.
🤖 Prompt for AI agents
In internal/cluster/providers/k3d/manager.go around line 178, review and complete this code-review fix: fmt.Printf success/warning messages in DeleteCluster fallback path bypass pterm.
What the draft fix changed: In `DeleteCluster`'s Docker cleanup fallback path, the pre-cleanup verbose message was converted to `pterm.Warning.Printf("k3d delete failed, attempting direct Docker cleanup for cluster %s: %v\n", name, err)` and the post-cleanup success message was converted to `pterm.Success.Printf("Cluster %s removed via direct Docker cleanup\n", name)` (dropped the manual "✓ " prefix since `pterm.Success` renders its own success indicator). Risk: pterm's default writer target and exact prefix formatting were assumed consistent with other usages in the package (e.g. operations.go); if this package configures a custom pterm writer/silence toggle elsewhere, verify these new calls respect it identically to existing pterm calls.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
Closes 3 review findings in
internal/cluster/providers/k3d/manager.go.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
internal/cluster/providers/k3d/manager.go:73internal/cluster/providers/k3d/manager.go:92internal/cluster/providers/k3d/manager.go:178What 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:
91c75a3f-4f00-4dd6-9120-a00ab67ad34dMerging 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.