Skip to content

fix(OPENFRAM-007): 3 review findings in manager.go #351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −13
Draft

fix(OPENFRAM-007): 3 review findings in manager.go #351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions internal/cluster/providers/k3d/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/flamingo-stack/openframe-cli/internal/cluster/models"
"github.com/flamingo-stack/openframe-cli/internal/shared/executor"
"github.com/pterm/pterm"
"k8s.io/client-go/rest"
)

Expand Down Expand Up @@ -68,7 +69,7 @@ func (m *K3dManager) CreateCluster(ctx context.Context, config models.ClusterCon
// This must be done before cluster creation as it affects the Docker/WSL host
if err := m.increaseInotifyLimits(ctx); err != nil {
if m.verbose {
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

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

}
Expand All @@ -82,22 +83,22 @@ func (m *K3dManager) CreateCluster(ctx context.Context, config models.ClusterCon

if m.verbose {
if configContent, err := os.ReadFile(configFile); err == nil { // #nosec G304 -- reads a temp config file this process just created
fmt.Printf("DEBUG: Config file content for %s:\n%s\n", config.Name, string(configContent))
pterm.Debug.Printf("Config file content for %s:\n%s\n", config.Name, string(configContent))
}
}

// 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)

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

pterm.Warning.Printf("Could not prepare kubeconfig directory: %v\n", err)
}
// Don't fail - k3d will create it, but log the warning
}

// Clean up any stale lock files that might prevent k3d from updating kubeconfig
if err := m.cleanupStaleLockFiles(ctx); err != nil {
if m.verbose {
fmt.Printf("Warning: Could not cleanup stale lock files: %v\n", err)
pterm.Warning.Printf("Could not cleanup stale lock files: %v\n", err)
}
// Don't fail - this is not critical
}
Expand All @@ -122,7 +123,7 @@ func (m *K3dManager) CreateCluster(ctx context.Context, config models.ClusterCon
// This is necessary because k3d creates ~/.kube/config with root ownership when run with sudo
if err := m.fixKubeconfigPermissions(ctx); err != nil {
if m.verbose {
fmt.Printf("Warning: Could not fix kubeconfig permissions: %v\n", err)
pterm.Warning.Printf("Could not fix kubeconfig permissions: %v\n", err)
}
// Don't fail - this is not critical, just log the warning
}
Expand All @@ -131,7 +132,7 @@ func (m *K3dManager) CreateCluster(ctx context.Context, config models.ClusterCon
// This is critical because lock files may have been created with root ownership
if err := m.cleanupStaleLockFiles(ctx); err != nil {
if m.verbose {
fmt.Printf("Warning: Could not cleanup lock files after permission fix: %v\n", err)
pterm.Warning.Printf("Could not cleanup lock files after permission fix: %v\n", err)
}
// Don't fail - this is not critical
}
Expand Down Expand Up @@ -188,15 +189,15 @@ func (m *K3dManager) DeleteCluster(ctx context.Context, name string, clusterType
// 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
}
Comment on lines 189 to 203

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down Expand Up @@ -233,15 +234,15 @@ func (m *K3dManager) forceCleanupDockerContainers(ctx context.Context, clusterNa
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
Expand Down Expand Up @@ -523,14 +524,14 @@ func (m *K3dManager) increaseInotifyLimitsFor(ctx context.Context, goos string)
}

if m.verbose {
fmt.Printf("✓ Increased inotify limits in WSL (max_user_watches=%d, max_user_instances=%d)\n",
pterm.Success.Printf("Increased inotify limits in WSL (max_user_watches=%d, max_user_instances=%d)\n",
maxUserWatches, maxUserInstances)
}
default: // linux
// Skip the privileged write when the current limits already suffice.
if m.inotifyLimitsSufficient(ctx, maxUserWatches, maxUserInstances) {
if m.verbose {
fmt.Println("✓ inotify limits already sufficient")
pterm.Success.Println("inotify limits already sufficient")
}
return nil
}
Expand All @@ -548,7 +549,7 @@ func (m *K3dManager) increaseInotifyLimitsFor(ctx context.Context, goos string)
}

if m.verbose {
fmt.Printf("✓ Increased inotify limits (max_user_watches=%d, max_user_instances=%d)\n",
pterm.Success.Printf("Increased inotify limits (max_user_watches=%d, max_user_instances=%d)\n",
maxUserWatches, maxUserInstances)
}
}
Expand Down
Loading