Repository navigation
fix(FLEETMDM-002-2): CU-86aknprf9 11 review findings across 8 files - #413
flamingo[bot] wants to merge 8 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -54,7 +54,7 @@ func syncOperationPatch(prune bool) string { | |||
| func (m *Manager) RefreshAndSync(ctx context.Context, prune bool) error { | |||
| if m.dynamicClient == nil { | |||
There was a problem hiding this comment.
🦩 🟠 initKubernetesClients error returned bare without wrapping in RefreshAndSync
In RefreshAndSync, the m.initKubernetesClients() error path now wraps with fmt.Errorf("initializing kubernetes clients: %w", err) instead of returning err bare, matching the suggested fix exactly.
🤖 Prompt for AI agents
In internal/chart/providers/argocd/sync.go around line 55, review and complete this code-review fix: initKubernetesClients error returned bare without wrapping in RefreshAndSync.
What the draft fix changed: In `RefreshAndSync`, the `m.initKubernetesClients()` error path now wraps with `fmt.Errorf("initializing kubernetes clients: %w", err)` instead of returning `err` bare, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -85,7 +85,7 @@ func (m *Manager) RefreshAndSync(ctx context.Context, prune bool) error { | |||
| // 2) Don't clobber an operation already in flight — setting .operation over a | |||
| // running one is racy; wait briefly, then refuse rather than stomp it. | |||
| if err := m.ensureNoRunningOperation(ctx, budget); err != nil { | |||
There was a problem hiding this comment.
🦩 🔵 ensureNoRunningOperation error returned bare without wrapping
In RefreshAndSync, the m.ensureNoRunningOperation(ctx, budget) error path now wraps with fmt.Errorf("waiting for in-flight operation to clear: %w", err) instead of returning err bare, matching the suggested fix exactly.
🤖 Prompt for AI agents
In internal/chart/providers/argocd/sync.go around line 87, review and complete this code-review fix: ensureNoRunningOperation error returned bare without wrapping.
What the draft fix changed: In `RefreshAndSync`, the `m.ensureNoRunningOperation(ctx, budget)` error path now wraps with `fmt.Errorf("waiting for in-flight operation to clear: %w", err)` instead of returning `err` bare, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -327,7 +327,7 @@ func (m *Manager) SyncApplications(ctx context.Context, names []string) error { | |||
| func (m *Manager) syncApplicationsByName(ctx context.Context, names []string, prune bool) (patched, failed int, firstErr error) { | |||
| if m.dynamicClient == nil { | |||
There was a problem hiding this comment.
🦩 🔵 syncApplicationsByName propagates initKubernetesClients error unwrapped
In syncApplicationsByName, the m.initKubernetesClients() error path now wraps with fmt.Errorf("initializing kubernetes clients: %w", err) instead of returning err bare, matching the suggested fix exactly.
🤖 Prompt for AI agents
In internal/chart/providers/argocd/sync.go around line 328, review and complete this code-review fix: syncApplicationsByName propagates initKubernetesClients error unwrapped.
What the draft fix changed: In `syncApplicationsByName`, the `m.initKubernetesClients()` error path now wraps with `fmt.Errorf("initializing kubernetes clients: %w", err)` instead of returning `err` bare, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -89,18 +90,18 @@ func (h *HelmInstaller) installLinux() error { | |||
| func (h *HelmInstaller) installVerified() error { | |||
| binDir, err := download.UserBinDir() | |||
There was a problem hiding this comment.
🦩 🔵 UserBinDir error returned bare without context wrapping
In installVerified (internal/chart/prerequisites/helm/helm.go), wrapped the bare return err from download.UserBinDir() with fmt.Errorf("resolving user bin dir: %w", err), matching the suggested fix exactly.
🤖 Prompt for AI agents
In internal/chart/prerequisites/helm/helm.go around line 90, review and complete this code-review fix: UserBinDir error returned bare without context wrapping.
What the draft fix changed: In installVerified (internal/chart/prerequisites/helm/helm.go), wrapped the bare `return err` from download.UserBinDir() with `fmt.Errorf("resolving user bin dir: %w", err)`, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — 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 used for user-facing progress output in installVerified (helm)
In installVerified, replaced fmt.Printf calls for the "Downloading verified helm..." and "Installed verified helm..." messages with pterm.Info.Printf and pterm.Success.Printf respectively, and added the github.com/pterm/pterm import (already used elsewhere in the project per the k3d installer reference), matching the suggested fix.
🤖 Prompt for AI agents
In internal/chart/prerequisites/helm/helm.go around line 98, review and complete this code-review fix: Raw fmt.Printf used for user-facing progress output in installVerified (helm).
What the draft fix changed: In installVerified, replaced `fmt.Printf` calls for the "Downloading verified helm..." and "Installed verified helm..." messages with `pterm.Info.Printf` and `pterm.Success.Printf` respectively, and added the `github.com/pterm/pterm` import (already used elsewhere in the project per the k3d installer reference), matching the suggested fix.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| Env: h.getHelmEnv(), | ||
| }) | ||
| if err != nil { | ||
| return false, err | ||
| return false, fmt.Errorf("checking install status of release %s in namespace %s: %w", releaseName, namespace, err) | ||
| } | ||
|
|
||
| releases := strings.Split(strings.TrimSpace(result.Stdout), "\n") |
There was a problem hiding this comment.
🦩 🔵 IsChartInstalled returns helm errors unwrapped, losing call-site context
In IsChartInstalled (internal/chart/providers/helm/manager.go), wrapped the raw error from h.executor.ExecuteWithOptions with fmt.Errorf("checking install status of release %s in namespace %s: %w", releaseName, namespace, err) before returning, matching the error-wrapping convention used elsewhere in the file (e.g. UninstallRelease, GetChartStatus). No other logic changed.
🤖 Prompt for AI agents
In internal/chart/providers/helm/manager.go around line 151, review and complete this code-review fix: IsChartInstalled returns helm errors unwrapped, losing call-site context.
What the draft fix changed: In IsChartInstalled (internal/chart/providers/helm/manager.go), wrapped the raw error from h.executor.ExecuteWithOptions with fmt.Errorf("checking install status of release %s in namespace %s: %w", releaseName, namespace, err) before returning, matching the error-wrapping convention used elsewhere in the file (e.g. UninstallRelease, GetChartStatus). No other logic changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -41,7 +41,11 @@ func ParseBackendURL(raw string) (BackendConfig, error) { | |||
| // the rendered HCL for their provider's backend type. | |||
| func (w *Workspace) WriteBackend(backendTF []byte) error { | |||
There was a problem hiding this comment.
🦩 🔵 WriteBackend returns bare os errors without wrapping
In WriteBackend (internal/cluster/providers/terraform/backend.go), wrapped the os.MkdirAll error with fmt.Errorf("creating terraform dir: %w", err) and the os.WriteFile error with fmt.Errorf("writing backend.tf: %w", err), matching the suggested fix exactly; fmt was already imported.
🤖 Prompt for AI agents
In internal/cluster/providers/terraform/backend.go around line 42, review and complete this code-review fix: WriteBackend returns bare os errors without wrapping.
What the draft fix changed: In WriteBackend (internal/cluster/providers/terraform/backend.go), wrapped the os.MkdirAll error with fmt.Errorf("creating terraform dir: %w", err) and the os.WriteFile error with fmt.Errorf("writing backend.tf: %w", err), matching the suggested fix exactly; fmt was already imported.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -50,12 +50,12 @@ func EstimateMonthlyCost(ctx context.Context, execer sharedexec.CommandExecutor, | |||
| } | |||
| tmp, err := os.MkdirTemp("", "openframe-infracost-*") | |||
There was a problem hiding this comment.
🦩 🔵 EstimateMonthlyCost swallows context on several os/json errors
In EstimateMonthlyCost (internal/cluster/providers/terraform/cost.go), wrapped the os.MkdirTemp error with "creating temp dir: %w" and the os.WriteFile error with "writing plan json: %w" using fmt.Errorf, matching the style already used for the infracost breakdown and json unmarshal errors in the same function.
🤖 Prompt for AI agents
In internal/cluster/providers/terraform/cost.go around line 51, review and complete this code-review fix: EstimateMonthlyCost swallows context on several os/json errors.
What the draft fix changed: In EstimateMonthlyCost (internal/cluster/providers/terraform/cost.go), wrapped the os.MkdirTemp error with "creating temp dir: %w" and the os.WriteFile error with "writing plan json: %w" using fmt.Errorf, matching the style already used for the infracost breakdown and json unmarshal errors in the same function.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| continue | ||
| } | ||
| if err != nil { | ||
| return err | ||
| return fmt.Errorf("reading %s: %w", name, err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(dstDir, name), data, 0o600); err != nil { // #nosec G703 -- dstDir is our own MkdirTemp dir; srcDir is CLI-managed under ~/.openframe | ||
| return err | ||
| return fmt.Errorf("writing %s: %w", name, err) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🦩 🔵 CopyPlanInputs returns bare os error without context wrapping
In CopyPlanInputs (internal/cluster/providers/terraform/preview.go), wrapped the os.ReadFile error with fmt.Errorf("reading %s: %w", name, err) and the os.WriteFile error with fmt.Errorf("writing %s: %w", name, err), adding the "fmt" import, so both failure paths identify the filename and operation at the call boundary.
🤖 Prompt for AI agents
In internal/cluster/providers/terraform/preview.go around line 19, review and complete this code-review fix: CopyPlanInputs returns bare os error without context wrapping.
What the draft fix changed: In CopyPlanInputs (internal/cluster/providers/terraform/preview.go), wrapped the os.ReadFile error with fmt.Errorf("reading %s: %w", name, err) and the os.WriteFile error with fmt.Errorf("writing %s: %w", name, err), adding the "fmt" import, so both failure paths identify the filename and operation at the call boundary.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -209,7 +209,7 @@ func (d *GKEDiscoverer) Discover(ctx context.Context) (Result, error) { | |||
| } | |||
| var clusters []gkeCluster | |||
There was a problem hiding this comment.
🦩 🔵 describe() in gke.go returns unwrapped errors from json.Unmarshal in Discover's inline path
In GKEDiscoverer.Discover (internal/cluster/discovery/gke.go), the json.Unmarshal error-handling branch for the clusters list now includes the underlying error via %v in the warning message: fmt.Sprintf("%s: unparseable clusters list: %v", project, err), matching the suggested fix exactly. The EKSDiscoverer findings mentioned in the finding text live in a different file (eks.go) which was not provided, so no change was made there; only the gke.go occurrence was addressed as scoped by the task.
🤖 Prompt for AI agents
In internal/cluster/discovery/gke.go around line 210, review and complete this code-review fix: describe() in gke.go returns unwrapped errors from json.Unmarshal in Discover's inline path.
What the draft fix changed: In GKEDiscoverer.Discover (internal/cluster/discovery/gke.go), the json.Unmarshal error-handling branch for the clusters list now includes the underlying error via `%v` in the warning message: `fmt.Sprintf("%s: unparseable clusters list: %v", project, err)`, matching the suggested fix exactly. The EKSDiscoverer findings mentioned in the finding text live in a different file (eks.go) which was not provided, so no change was made there; only the gke.go occurrence was addressed as scoped by the task.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 11 review findings across 8 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
internal/chart/providers/argocd/sync.go:55internal/chart/providers/argocd/sync.go:87internal/chart/providers/argocd/sync.go:328internal/chart/prerequisites/helm/helm.go:90internal/chart/prerequisites/helm/helm.go:98internal/shared/download/verify.go:268internal/chart/providers/helm/manager.go:151internal/cluster/providers/terraform/backend.go:42internal/cluster/providers/terraform/cost.go:51internal/cluster/providers/terraform/preview.go:19internal/cluster/discovery/gke.go:210What 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:
11664740-3810-4fc3-b2cc-8de9b26fe218Merging 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-86aknprf9 OpenFrame CLI code duplication and manager fixes (13 PRs)