Skip to content

fix(OPENFRAM-007): CU-86akbhhau 5 review findings across 4 files #379

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 4 commits into
base: main
Choose a base branch
from
+22 −16
Draft

fix(OPENFRAM-007): CU-86akbhhau 5 review findings across 4 files #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5744850
fix(OPENFRAM-007): 5 review findings across 4 files
flamingo[bot] Sep 14, 2026
9f14fcc
fix(OPENFRAM-007): 5 review findings across 4 files
flamingo[bot] Sep 14, 2026
3accfdc
fix(OPENFRAM-007): 5 review findings across 4 files
flamingo[bot] Sep 14, 2026
58ebfeb
fix(OPENFRAM-007): 5 review findings across 4 files
flamingo[bot] Sep 14, 2026
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
2 changes: 1 addition & 1 deletion internal/chart/providers/helm/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func (h *HelmManager) InstallArgoCDWithProgress(ctx context.Context, config conf
var spinner *uispinner.Spinner

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 Raw pterm.Info.Println/Printf calls bypass silent-mode gating in HelmManager

In InstallArgoCDWithProgress (internal/chart/providers/helm/manager.go), changed the else branch that unconditionally called pterm.Info.Println("Installing ArgoCD...") to else if !config.Silent, so the message is now only printed in non-interactive-but-not-silent mode, and fully suppressed when config.Silent is true — matching the suggested fix exactly and respecting the --silent flag per OPENFRAM-007.

🤖 Prompt for AI agents 
In internal/chart/providers/helm/manager.go around line 313, review and complete this code-review fix: Raw pterm.Info.Println/Printf calls bypass silent-mode gating in HelmManager.
What the draft fix changed: In `InstallArgoCDWithProgress` (internal/chart/providers/helm/manager.go), changed the `else` branch that unconditionally called `pterm.Info.Println("Installing ArgoCD...")` to `else if !config.Silent`, so the message is now only printed in non-interactive-but-not-silent mode, and fully suppressed when `config.Silent` is true — matching the suggested fix exactly and respecting the --silent flag per OPENFRAM-007.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

if !config.Silent && !config.NonInteractive {
spinner = uispinner.Start("Installing ArgoCD...")
} else {
} else if !config.Silent {
pterm.Info.Println("Installing ArgoCD...")
}

Expand Down
5 changes: 4 additions & 1 deletion internal/chart/services/appofapps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ func (a *AppOfApps) Install(ctx context.Context, config config.ChartInstallConfi
// wording read as if it reflected the cluster's current ref, which made a
// dry-run against a cluster on another ref confusing (verification report,
// minor observation).
pterm.Info.Printf("Deploying ref '%s'...\n", appConfig.GitHubBranch)

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 AppOfApps.Install uses pterm.Info.Printf directly instead of a silence-aware wrapper for the ref message

In AppOfApps.Install (internal/chart/services/appofapps.go), wrapped the pterm.Info.Printf("Deploying ref '%s'...\n", ...) call with if !config.Silent { ... }, matching the existing config.Silent check already used for the clone spinner in the same function. This makes the ref message respect --silent consistently with the rest of the file's convention, without introducing a new shared silence-aware wrapper module (none exists in the given file to extract to, and creating one would exceed the "minimal fix in this file" scope). Risk: if other call sites rely on this exact message being unconditionally printed (e.g. tests asserting on stdout), this would break them; a complete fix per the finding's broader framing would add a genuine silence-aware output abstraction used repo-wide, which is out of scope for a single-file fix.

🤖 Prompt for AI agents 
In internal/chart/services/appofapps.go around line 54, review and complete this code-review fix: AppOfApps.Install uses pterm.Info.Printf directly instead of a silence-aware wrapper for the ref message.
What the draft fix changed: In AppOfApps.Install (internal/chart/services/appofapps.go), wrapped the `pterm.Info.Printf("Deploying ref '%s'...\n", ...)` call with `if !config.Silent { ... }`, matching the existing `config.Silent` check already used for the clone spinner in the same function. This makes the ref message respect `--silent` consistently with the rest of the file's convention, without introducing a new shared silence-aware wrapper module (none exists in the given file to extract to, and creating one would exceed the "minimal fix in this file" scope). Risk: if other call sites rely on this exact message being unconditionally printed (e.g. tests asserting on stdout), this would break them; a complete fix per the finding's broader framing would add a genuine silence-aware output abstraction used repo-wide, which is out of scope for a single-file fix.
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

if !config.Silent {
pterm.Info.Printf("Deploying ref '%s'...\n", appConfig.GitHubBranch)
}

// Clone the repository to a temporary directory. On a cold cache this is a
// full clone over the network and used to run without any indicator.
Expand Down Expand Up @@ -120,3 +122,4 @@ func (a *AppOfApps) IsInstalled(ctx context.Context, namespace string) (bool, er
func (a *AppOfApps) GetStatus(ctx context.Context, namespace string) (models.ChartInfo, error) {
return a.helmManager.GetChartStatus(ctx, "app-of-apps", namespace)
}

29 changes: 16 additions & 13 deletions internal/cluster/providers/k3d/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"strings"
"time"

"github.com/pterm/pterm"

sharedconfig "github.com/flamingo-stack/openframe-cli/internal/shared/config"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -46,7 +48,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
}

if m.verbose {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 verify.go uses raw fmt.Printf/fmt.Println for user-facing verbose output instead of pterm/ui helpers

Replaced raw fmt.Printf/fmt.Println calls in verifyClusterReachable (lines with context switch confirmation, TLS bypass notice, host:port extraction warning, and the node-readiness retry loop) with pterm.Success/pterm.Info/pterm.Warning printers (pterm.Success.Printfln, pterm.Info.Printfln, pterm.Warning.Printfln, etc.), all still gated by if m.verbose. This routes output through pterm's writer so it respects pterm's global output redirection used in tests, but I did not have visibility into a project-specific internal/shared/ui wrapper, so I used the pterm package directly rather than inventing an ui helper API that may not exist. --silent/--plain flag wiring is not addressed here since that logic is outside this file's visibility.

🤖 Prompt for AI agents 
In internal/cluster/providers/k3d/verify.go around line 48, review and complete this code-review fix: verify.go uses raw fmt.Printf/fmt.Println for user-facing verbose output instead of pterm/ui helpers.
What the draft fix changed: Replaced raw fmt.Printf/fmt.Println calls in verifyClusterReachable (lines with context switch confirmation, TLS bypass notice, host:port extraction warning, and the node-readiness retry loop) with pterm.Success/pterm.Info/pterm.Warning printers (pterm.Success.Printfln, pterm.Info.Printfln, pterm.Warning.Printfln, etc.), all still gated by `if m.verbose`. This routes output through pterm's writer so it respects pterm's global output redirection used in tests, but I did not have visibility into a project-specific internal/shared/ui wrapper, so I used the pterm package directly rather than inventing an ui helper API that may not exist. --silent/--plain flag wiring is not addressed here since that logic is outside this file's visibility.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer

fmt.Printf("✓ Switched kubectl context to %s\n", contextName)
pterm.Success.Printfln("Switched kubectl context to %s", contextName)
}

// Build rest.Config from the loaded Kubeconfig
Expand All @@ -66,7 +68,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
restConfig = sharedconfig.ApplyInsecureTLSConfig(restConfig)

if m.verbose {
fmt.Println("✓ TLS verification bypassed for local k3d cluster (Insecure=true, auth preserved)")
pterm.Success.Println("TLS verification bypassed for local k3d cluster (Insecure=true, auth preserved)")
}

// --- PHASE 2: Verify Network Connectivity and Update Endpoint ---
Expand All @@ -75,7 +77,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
host, port, err := extractHostPort(restConfig.Host)
if err != nil {
if m.verbose {
fmt.Printf("Warning: Could not extract host:port from %s: %v\n", restConfig.Host, err)
pterm.Warning.Printfln("Could not extract host:port from %s: %v", restConfig.Host, err)
}
// Default to 127.0.0.1:6550 for k3d
host = "127.0.0.1"
Expand Down Expand Up @@ -105,7 +107,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
var lastErr error

if m.verbose {
fmt.Println("Waiting for cluster API and nodes to be reachable...")
pterm.Info.Println("Waiting for cluster API and nodes to be reachable...")
}

for i := 0; i < maxRetries; i++ {
Expand All @@ -123,7 +125,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
if isTemporaryError(err) {
lastErr = err
if m.verbose {
fmt.Printf(" Cluster not ready yet (attempt %d/%d): %v\n", i+1, maxRetries, err)
pterm.Info.Printfln(" Cluster not ready yet (attempt %d/%d): %v", i+1, maxRetries, err)
}
time.Sleep(retryDelay)
continue
Expand All @@ -136,7 +138,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
if len(nodes.Items) == 0 {
lastErr = fmt.Errorf("no nodes found in cluster")
if m.verbose {
fmt.Printf(" No nodes found yet (attempt %d/%d), waiting...\n", i+1, maxRetries)
pterm.Info.Printfln(" No nodes found yet (attempt %d/%d), waiting...", i+1, maxRetries)
}
time.Sleep(retryDelay)
continue
Expand All @@ -157,15 +159,15 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
// Success condition: Nodes exist and at least one is ready
if readyCount > 0 {
if m.verbose {
fmt.Printf(" Found %d ready node(s) out of %d total\n", readyCount, len(nodes.Items))
fmt.Println("✓ Cluster API and nodes are ready.")
pterm.Info.Printfln(" Found %d ready node(s) out of %d total", readyCount, len(nodes.Items))
pterm.Success.Println("Cluster API and nodes are ready.")
}
return restConfig, nil
}

lastErr = fmt.Errorf("no nodes in Ready state (found %d nodes, 0 ready)", len(nodes.Items))
if m.verbose {
fmt.Printf(" Nodes exist but none are Ready yet (attempt %d/%d), waiting...\n", i+1, maxRetries)
pterm.Info.Printfln(" Nodes exist but none are Ready yet (attempt %d/%d), waiting...", i+1, maxRetries)
}
time.Sleep(retryDelay)
}
Expand Down Expand Up @@ -194,7 +196,7 @@ func (m *K3dManager) waitForTCPPort(ctx context.Context, host string, port strin
address := net.JoinHostPort(host, port)

if m.verbose {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 waitForTCPPort emits raw fmt.Printf for progress instead of using pterm

Replaced fmt.Printf calls in waitForTCPPort (the "Waiting for TCP port..." message, the "TCP port %s is open" success message, and the retry-attempt message) with pterm.Info.Printfln and pterm.Success.Printfln, preserving the existing if m.verbose guards. Also updated cleanupStaleLockFiles's fmt.Println to pterm.Success.Println for consistency, since it exhibited the same raw-fmt pattern called out generally in finding 1. Same caveat as above: used pterm directly since no internal/shared/ui module was visible to import safely.

🤖 Prompt for AI agents 
In internal/cluster/providers/k3d/verify.go around line 196, review and complete this code-review fix: waitForTCPPort emits raw fmt.Printf for progress instead of using pterm.
What the draft fix changed: Replaced fmt.Printf calls in waitForTCPPort (the "Waiting for TCP port..." message, the "TCP port %s is open" success message, and the retry-attempt message) with pterm.Info.Printfln and pterm.Success.Printfln, preserving the existing `if m.verbose` guards. Also updated cleanupStaleLockFiles's fmt.Println to pterm.Success.Println for consistency, since it exhibited the same raw-fmt pattern called out generally in finding 1. Same caveat as above: used pterm directly since no internal/shared/ui module was visible to import safely.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer

fmt.Printf("Waiting for TCP port %s to be available...\n", address)
pterm.Info.Printfln("Waiting for TCP port %s to be available...", address)
}

var lastErr error
Expand All @@ -212,14 +214,14 @@ func (m *K3dManager) waitForTCPPort(ctx context.Context, host string, port strin
if err == nil {
_ = conn.Close()
if m.verbose {
fmt.Printf("✓ TCP port %s is open\n", address)
pterm.Success.Printfln("TCP port %s is open", address)
}
return nil
}

lastErr = err
if m.verbose {
fmt.Printf(" TCP port not ready yet (attempt %d/%d): %v\n", i+1, maxRetries, err)
pterm.Info.Printfln(" TCP port not ready yet (attempt %d/%d): %v", i+1, maxRetries, err)
}
time.Sleep(retryDelay)
}
Expand Down Expand Up @@ -301,8 +303,9 @@ func (m *K3dManager) cleanupStaleLockFiles(ctx context.Context) error {
}

if m.verbose {
fmt.Println("✓ Cleaned up stale kubeconfig lock files")
pterm.Success.Println("Cleaned up stale kubeconfig lock files")
}

return nil
}

2 changes: 1 addition & 1 deletion internal/cluster/ui/wizard_steps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ func (ws *WizardSteps) ConfirmConfiguration(config models.ClusterConfig) (bool,
if i == 0 {
continue // Skip header
}
println(row[0] + ": " + row[1])
pterm.Info.Printf("%s: %s\n", row[0], row[1])
}
}

Comment on lines 274 to 280

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 Raw println() used as fallback instead of pterm-based output

Replaced the raw println(row[0] + ": " + row[1]) builtin call in ConfirmConfiguration's fallback branch (used when renderConfigurationTable fails) with pterm.Info.Printf("%s: %s\n", row[0], row[1]), matching the suggested fix exactly so the fallback path goes through pterm and respects --silent/--plain and test writer redirection.

🤖 Prompt for AI agents 
In internal/cluster/ui/wizard_steps.go around line 271, review and complete this code-review fix: Raw println() used as fallback instead of pterm-based output.
What the draft fix changed: Replaced the raw `println(row[0] + ": " + row[1])` builtin call in ConfirmConfiguration's fallback branch (used when renderConfigurationTable fails) with `pterm.Info.Printf("%s: %s\n", row[0], row[1])`, matching the suggested fix exactly so the fallback path goes through pterm and respects --silent/--plain and test writer redirection.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

Expand Down
Loading