From 3747657f8386630dc6fef31dfa84f36c3904a46a Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Mon, 13 Jul 2026 12:51:41 +0300 Subject: [PATCH 01/11] fix(cli): address 0.4.8 verification findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Blocker: disable dex in the embedded ArgoCD values. The argo-cd chart 10.1.3 defaults dex.enabled=true, and dexidp/dex:v2.45.1's arm64 image intermittently SIGSEGVs under emulation on Apple Silicon before its first log line -> CrashLoopBackOff -> the 7m `helm --wait` never completes -> fresh installs fail. OpenFrame's login uses the local developer account, never dex. Verified with `helm template`: 0 dex-server objects with our values vs 3 at the chart default; core components unchanged. - Track stall per application instead of one global fingerprint. The old fingerprint covered the whole not-ready set, so a single neighbour oscillating Missing<->OutOfSync reset the 90s timer every tick while a genuinely stuck app sat Healthy+OutOfSync and never accrued stall time — the flagship N3 fix never fired in exactly the scenario it was written for. stallTracker times each app independently; a stuck app is now detected regardless of noisy neighbours. - Assert the deployed ref before declaring success. `app install --ref ` writes the flattened repository.branch, but a branch whose chart predates that key ignores it — children render from main, everything goes Healthy+Synced, and the CLI printed "17/17 ready ... SUCCESS" for a deployment of the wrong ref. verifyRefPinning compares OSS-repo children's targetRevision against the requested ref and fails loudly on a mismatch; it skips default refs (main/master/HEAD), ignores foreign-repo children, and normalizes URLs with embedded credentials. - Map a pending helm release to an actionable hint. A release wedged in pending-* by an interrupted operation ("another operation is in progress") got the generic "wait and retry" timeout hint — but retrying hits the same pending release. Match it before the timeout case and point at `helm rollback`, noting pending releases need `helm list -a` to be seen. - Stop dumping raw helm output under --verbose. The full helm stdout, the `helm list` JSON, and the 150-line `helm status` inventory (chart NOTES twice) buried the useful lines; stderr warnings on the install path are still shown. --- .../chart/providers/argocd/argocd-values.yaml | 15 ++- internal/chart/providers/argocd/refassert.go | 105 +++++++++++++++ .../chart/providers/argocd/refassert_test.go | 86 +++++++++++++ internal/chart/providers/argocd/stall.go | 69 +++++++--- internal/chart/providers/argocd/stall_test.go | 121 ++++++++++++++---- .../chart/providers/argocd/values_test.go | 25 +++- internal/chart/providers/argocd/wait.go | 57 +++++---- internal/chart/providers/helm/argocd_wait.go | 27 ++-- internal/chart/providers/helm/manager.go | 21 +-- internal/shared/errors/friendly.go | 6 + .../shared/errors/friendly_upstream_test.go | 21 +++ 11 files changed, 449 insertions(+), 104 deletions(-) create mode 100644 internal/chart/providers/argocd/refassert.go create mode 100644 internal/chart/providers/argocd/refassert_test.go diff --git a/internal/chart/providers/argocd/argocd-values.yaml b/internal/chart/providers/argocd/argocd-values.yaml index 3c8153d6..a25c90c2 100644 --- a/internal/chart/providers/argocd/argocd-values.yaml +++ b/internal/chart/providers/argocd/argocd-values.yaml @@ -112,13 +112,14 @@ redis: dex: - resources: - requests: - cpu: 50m - memory: 64Mi - limits: - cpu: 100m - memory: 128Mi + # Dex (SSO) is disabled: the chart defaults it on (dex.enabled: true), but + # OpenFrame's local/oss-tenant login uses the local `developer` account + # configured above, never dex. Beyond being dead weight on local k3d, the + # dexidp/dex:v2.45.1 arm64 image intermittently SIGSEGVs under emulation on + # Apple Silicon (exit 139) before its first log line -> CrashLoopBackOff -> + # the 7m `helm --wait` never completes -> fresh installs fail. Disabling it + # removes the blocker; re-enable and pin a known-good image if SSO is needed. + enabled: false applicationSet: diff --git a/internal/chart/providers/argocd/refassert.go b/internal/chart/providers/argocd/refassert.go new file mode 100644 index 00000000..67947bac --- /dev/null +++ b/internal/chart/providers/argocd/refassert.go @@ -0,0 +1,105 @@ +package argocd + +import ( + "fmt" + "sort" + "strings" +) + +// refMismatch is one child application that ArgoCD is tracking at a different +// git revision than the ref the user asked the CLI to deploy. +type refMismatch struct { + App string + Want string // requested ref + Got string // the app's actual spec.source.targetRevision +} + +// defaultRefs are the branch names indistinguishable from "no pinning": if the +// user requests one of these and a legacy chart ignores the pin, the children +// land on main anyway — the same place — so there is nothing to warn about. +var defaultRefs = map[string]bool{"": true, "main": true, "master": true, "head": true} + +// verifyRefPinning reports the OSS-repo child applications whose targetRevision +// does not match the requested ref. +// +// It exists for the V3 silent-failure: `app install --ref ` writes +// the flattened repository.branch, but a branch whose chart predates that key +// ignores it — children render from main, the CLI waits, everything goes +// Healthy+Synced, and "17/17 ready … SUCCESS" is printed for a deployment of +// main, not the requested ref. Comparing what ArgoCD actually tracks against +// what was asked turns that into a loud, specific failure. +// +// Only children pointing at repoURL are considered: a child that legitimately +// sources a different repository (third-party) has its own revision and must +// not be flagged. A child with an empty targetRevision is skipped (unknowable), +// as is any request for a default ref (see defaultRefs). +func verifyRefPinning(apps []Application, repoURL, requestedRef string) []refMismatch { + if defaultRefs[strings.ToLower(strings.TrimSpace(requestedRef))] { + return nil + } + want := normalizeRef(requestedRef) + repo := normalizeRepoURL(repoURL) + + var out []refMismatch + for _, app := range apps { + if repo != "" && normalizeRepoURL(app.RepoURL) != repo { + continue // different repository — not ours to judge + } + got := strings.TrimSpace(app.TargetRevision) + if got == "" { + continue // no declared revision — nothing to compare + } + if normalizeRef(got) != want { + out = append(out, refMismatch{App: app.Name, Want: requestedRef, Got: got}) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].App < out[j].App }) + return out +} + +// normalizeRef trims a git ref for comparison. Branch and tag references are +// compared by their short name; a "v"-prefixed tag and its bare form are NOT +// unified (they are distinct git refs). +func normalizeRef(ref string) string { + ref = strings.TrimSpace(ref) + ref = strings.TrimPrefix(ref, "refs/heads/") + ref = strings.TrimPrefix(ref, "refs/tags/") + return ref +} + +// normalizeRepoURL canonicalizes a git remote for comparison: lowercased, no +// scheme, no embedded credentials, no trailing ".git" or slash. Enough to match +// "https://user:tok@github.com/org/repo.git" against "github.com/org/repo". +func normalizeRepoURL(u string) string { + u = strings.TrimSpace(strings.ToLower(u)) + if u == "" { + return "" + } + if i := strings.Index(u, "://"); i >= 0 { + u = u[i+3:] + } + u = strings.TrimPrefix(u, "git@") + if at := strings.LastIndex(u, "@"); at >= 0 { // strip user:token@ + u = u[at+1:] + } + u = strings.ReplaceAll(u, ":", "/") // scp-style host:org/repo + u = strings.TrimSuffix(u, "/") + u = strings.TrimSuffix(u, ".git") + return u +} + +// refMismatchError renders the mismatches into a loud, actionable error. The +// workloads are running — but not from the ref the user asked for, so the +// requested operation did not do what it said. +func refMismatchError(requestedRef string, m []refMismatch) error { + var b strings.Builder + fmt.Fprintf(&b, "requested ref %q was NOT deployed: this branch's chart predates ref pinning, "+ + "so its child applications ignored the pin and ArgoCD is tracking a different revision.\n", + requestedRef) + b.WriteString("The workloads are running, but they do NOT reflect the requested ref:\n") + for _, x := range m { + fmt.Fprintf(&b, " - %s is on %q, not %q\n", x.App, x.Got, x.Want) + } + b.WriteString("Use a branch whose chart reads repository.branch, or pin these applications' targetRevision by hand.") + return fmt.Errorf("%s", b.String()) +} diff --git a/internal/chart/providers/argocd/refassert_test.go b/internal/chart/providers/argocd/refassert_test.go new file mode 100644 index 00000000..af8ccce8 --- /dev/null +++ b/internal/chart/providers/argocd/refassert_test.go @@ -0,0 +1,86 @@ +package argocd + +import ( + "strings" + "testing" +) + +const ossRepo = "https://github.com/flamingo-stack/openframe-oss-tenant" + +// TestVerifyRefPinning_LegacyBranchSilentlyDeploysMain is the V3 case: the user +// asked for a legacy branch, its chart ignored repository.branch, and ArgoCD is +// tracking main. That must be reported — the CLI otherwise prints SUCCESS for a +// deployment of the wrong ref. +func TestVerifyRefPinning_LegacyBranchSilentlyDeploysMain(t *testing.T) { + apps := []Application{ + {Name: "openframe-api", RepoURL: ossRepo, TargetRevision: "main"}, + {Name: "openframe-ui", RepoURL: ossRepo, TargetRevision: "main"}, + } + mm := verifyRefPinning(apps, ossRepo, "feature/configuration-updates") + if len(mm) != 2 { + t.Fatalf("both children on main must be flagged, got %d: %+v", len(mm), mm) + } + err := refMismatchError("feature/configuration-updates", mm) + for _, want := range []string{"predates ref pinning", "openframe-api", `on "main"`, "feature/configuration-updates"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must contain %q; got:\n%s", want, err.Error()) + } + } +} + +// TestVerifyRefPinning_ModernBranchMatches: a chart that honours the pin sets +// children's targetRevision to the requested ref — no mismatch. +func TestVerifyRefPinning_ModernBranchMatches(t *testing.T) { + apps := []Application{ + {Name: "openframe-api", RepoURL: ossRepo, TargetRevision: "feature/x"}, + {Name: "openframe-ui", RepoURL: ossRepo + ".git", TargetRevision: "feature/x"}, + } + if mm := verifyRefPinning(apps, ossRepo, "feature/x"); len(mm) != 0 { + t.Errorf("matching refs must not be flagged, got %+v", mm) + } +} + +// TestVerifyRefPinning_DefaultRefSkipped: requesting main (or empty) is +// indistinguishable from no pinning; do not flag. +func TestVerifyRefPinning_DefaultRefSkipped(t *testing.T) { + apps := []Application{{Name: "a", RepoURL: ossRepo, TargetRevision: "main"}} + for _, ref := range []string{"", "main", "master", "HEAD"} { + if mm := verifyRefPinning(apps, ossRepo, ref); len(mm) != 0 { + t.Errorf("default ref %q must be skipped, got %+v", ref, mm) + } + } +} + +// TestVerifyRefPinning_ForeignRepoIgnored: a child sourcing a different repo has +// its own revision and must not be judged against the OSS ref. +func TestVerifyRefPinning_ForeignRepoIgnored(t *testing.T) { + apps := []Application{ + {Name: "third-party", RepoURL: "https://charts.example.com/foo", TargetRevision: "1.2.3"}, + {Name: "openframe-api", RepoURL: ossRepo, TargetRevision: "feature/x"}, + } + if mm := verifyRefPinning(apps, ossRepo, "feature/x"); len(mm) != 0 { + t.Errorf("foreign-repo child must be ignored, got %+v", mm) + } +} + +// TestVerifyRefPinning_EmptyRevisionSkipped: a child with no declared revision +// is unknowable, not a mismatch. +func TestVerifyRefPinning_EmptyRevisionSkipped(t *testing.T) { + apps := []Application{{Name: "a", RepoURL: ossRepo, TargetRevision: ""}} + if mm := verifyRefPinning(apps, ossRepo, "feature/x"); len(mm) != 0 { + t.Errorf("empty targetRevision must be skipped, got %+v", mm) + } +} + +// TestVerifyRefPinning_RepoURLNormalization: credentials, scheme, and .git must +// not defeat the repo match (else every child looks "foreign" and nothing is +// checked). +func TestVerifyRefPinning_RepoURLNormalization(t *testing.T) { + apps := []Application{ + {Name: "openframe-api", RepoURL: "https://x-access-token:ghp_secret@github.com/flamingo-stack/openframe-oss-tenant.git", TargetRevision: "main"}, + } + mm := verifyRefPinning(apps, ossRepo, "feature/x") + if len(mm) != 1 { + t.Fatalf("credentialed/.git URL must still match the repo and be flagged, got %+v", mm) + } +} diff --git a/internal/chart/providers/argocd/stall.go b/internal/chart/providers/argocd/stall.go index 955bd178..a0b20e9e 100644 --- a/internal/chart/providers/argocd/stall.go +++ b/internal/chart/providers/argocd/stall.go @@ -1,9 +1,7 @@ package argocd import ( - "fmt" "sort" - "strings" "time" ) @@ -18,29 +16,66 @@ import ( // sync on exactly those stragglers (upgrade path, SyncStragglersOnStall) or // prints an actionable hint once (install path). -// stallAfter is how long the application set may stay bit-for-bit identical -// before the wait considers itself stalled. Long enough that a slow-but-live +// stallAfter is how long a SINGLE application may stay bit-for-bit identical +// before the wait considers it stalled. Long enough that a slow-but-live // rollout (images pulling, probes settling) keeps resetting it via status // transitions; short enough to leave time to act within the wait budget. const stallAfter = 90 * time.Second -// stallFingerprint captures the observable wait state: ready count plus the -// sorted not-ready list (names with their status strings). Any transition — -// an app appearing, progressing, or changing status — changes the fingerprint. -func stallFingerprint(ready int, notReady []string) string { - sorted := append([]string(nil), notReady...) - sort.Strings(sorted) - return fmt.Sprintf("%d|%s", ready, strings.Join(sorted, ",")) +// stallTracker records, per application, how long its (health, sync) state has +// been unchanged. +// +// It replaces a single global fingerprint over the whole not-ready set (V5): a +// global timer is reset by ANY transition anywhere in the set, so one noisy +// neighbour oscillating Missing<->OutOfSync every tick reset the 90s clock +// forever while a genuinely stuck app sat Healthy+OutOfSync, bit-for-bit +// identical, and never accrued stall time. Tracking each app independently lets +// a stuck app be detected regardless of what its neighbours do. +type stallTracker struct { + states map[string]stallEntry +} + +type stallEntry struct { + state string // "health|sync" + since time.Time // when the app first entered `state` +} + +func newStallTracker() *stallTracker { + return &stallTracker{states: make(map[string]stallEntry)} +} + +// observe records each application's current state, resetting an app's timer +// when its state changes and forgetting apps no longer present (so a +// reappearing app starts its clock fresh rather than inheriting a stale one). +func (s *stallTracker) observe(apps []Application, now time.Time) { + seen := make(map[string]bool, len(apps)) + for _, app := range apps { + seen[app.Name] = true + state := app.Health + "|" + app.Sync + if e, ok := s.states[app.Name]; !ok || e.state != state { + s.states[app.Name] = stallEntry{state: state, since: now} + } + } + for name := range s.states { + if !seen[name] { + delete(s.states, name) + } + } } -// outOfSyncStragglers returns the names of applications that are not ready -// solely because they are OutOfSync (health already fine). These are the ones -// a sync operation can actually move; apps with health problems are excluded — -// syncing them would mask the real failure. -func outOfSyncStragglers(apps []Application) []string { +// stalledStragglers returns the OutOfSync-but-Healthy applications whose state +// has been identical for at least stallAfter. These are the apps a sync can +// actually move (health is already fine, so syncing won't mask a real failure) +// AND that are genuinely stuck, judged per-app rather than across the set. +// +// Callers must observe() this same tick's apps first. +func (s *stallTracker) stalledStragglers(apps []Application, now time.Time) []string { var names []string for _, app := range apps { - if app.Sync == ArgoCDSyncOutOfSync && app.Health == ArgoCDHealthHealthy { + if app.Sync != ArgoCDSyncOutOfSync || app.Health != ArgoCDHealthHealthy { + continue + } + if e, ok := s.states[app.Name]; ok && now.Sub(e.since) >= stallAfter { names = append(names, app.Name) } } diff --git a/internal/chart/providers/argocd/stall_test.go b/internal/chart/providers/argocd/stall_test.go index f934b5f7..e2344463 100644 --- a/internal/chart/providers/argocd/stall_test.go +++ b/internal/chart/providers/argocd/stall_test.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" "testing" + "time" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -19,35 +20,109 @@ func labeledAppObj(name, health, sync string) *unstructured.Unstructured { return o } -func TestStallFingerprint(t *testing.T) { - // Order-insensitive: the same set in any order is the same fingerprint. - a := stallFingerprint(3, []string{"b (Sync: OutOfSync)", "a (Sync: OutOfSync)"}) - b := stallFingerprint(3, []string{"a (Sync: OutOfSync)", "b (Sync: OutOfSync)"}) - if a != b { - t.Errorf("fingerprint must be order-insensitive: %q vs %q", a, b) - } - // Any transition changes it: ready count, membership, or status. - if a == stallFingerprint(4, []string{"a (Sync: OutOfSync)", "b (Sync: OutOfSync)"}) { - t.Error("ready-count change must change the fingerprint") - } - if a == stallFingerprint(3, []string{"a (Sync: OutOfSync)", "b (Health: Degraded)"}) { - t.Error("status change must change the fingerprint") - } -} - -// TestOutOfSyncStragglers: only healthy-but-OutOfSync apps qualify — an app -// with health problems must not be auto-synced (it would mask a real failure). -func TestOutOfSyncStragglers(t *testing.T) { +// TestStallTracker_OnlyHealthyOutOfSyncQualify: only healthy-but-OutOfSync apps +// can be stalled stragglers — an app with health problems must not be +// auto-synced (it would mask a real failure). +func TestStallTracker_OnlyHealthyOutOfSyncQualify(t *testing.T) { apps := []Application{ {Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}, {Name: "ready", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncSynced}, - {Name: "broken", Health: "Degraded", Sync: ArgoCDSyncOutOfSync}, + {Name: "broken", Health: ArgoCDHealthDegraded, Sync: ArgoCDSyncOutOfSync}, {Name: "zoo", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}, } - got := outOfSyncStragglers(apps) + s := newStallTracker() + t0 := time.Unix(0, 0) + s.observe(apps, t0) + + // Before stallAfter elapses, nothing is stalled. + if got := s.stalledStragglers(apps, t0.Add(stallAfter-time.Second)); len(got) != 0 { + t.Errorf("nothing may be stalled before stallAfter, got %v", got) + } + // After it, only the healthy+OutOfSync apps qualify. + got := s.stalledStragglers(apps, t0.Add(stallAfter)) want := []string{"mongodb", "zoo"} - if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { - t.Errorf("stragglers = %v, want %v", got, want) + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("stalled stragglers = %v, want %v", got, want) + } +} + +// TestStallTracker_NoisyNeighbourDoesNotResetStuckApp is the V5 regression: a +// genuinely stuck app (Healthy+OutOfSync, never changing) must be detected even +// while a neighbour oscillates Missing<->OutOfSync every tick. The old global +// fingerprint reset its single timer on every neighbour transition, so the +// stuck app never accrued stall time and the wait rode to its full timeout. +func TestStallTracker_NoisyNeighbourDoesNotResetStuckApp(t *testing.T) { + s := newStallTracker() + base := time.Unix(0, 0) + + // The neighbour flaps on every tick; the stuck app never changes. Tick every + // 10s across more than stallAfter. + flapStates := []struct{ health, sync string }{ + {ArgoCDHealthMissing, ArgoCDSyncOutOfSync}, + {ArgoCDHealthProgressing, ArgoCDSyncOutOfSync}, + } + var last []string + for i := 0; i*10 <= int((stallAfter + 10*time.Second).Seconds()); i++ { + now := base.Add(time.Duration(i*10) * time.Second) + flap := flapStates[i%len(flapStates)] + apps := []Application{ + {Name: "argocd-apps", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}, // stone-cold stuck + {Name: "ingress-nginx", Health: flap.health, Sync: flap.sync}, // noisy + } + s.observe(apps, now) + last = s.stalledStragglers(apps, now) + } + + // The stuck app must be reported despite the neighbour's constant churn. + found := false + for _, n := range last { + if n == "argocd-apps" { + found = true + } + } + if !found { + t.Errorf("the stuck app must be detected regardless of a flapping neighbour; got %v", last) + } +} + +// TestStallTracker_ResetsOnStateChange: an app that genuinely progresses (its +// own state changes) restarts its clock and is not reported as stalled. +func TestStallTracker_ResetsOnStateChange(t *testing.T) { + s := newStallTracker() + t0 := time.Unix(0, 0) + + appsV1 := []Application{{Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}} + s.observe(appsV1, t0) + + // Just before it would stall, its sync flips (a real transition). + appsV2 := []Application{{Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncSynced}} + s.observe(appsV2, t0.Add(stallAfter-time.Second)) + // Then back to OutOfSync — the clock restarted at the transition. + appsV3 := appsV1 + s.observe(appsV3, t0.Add(stallAfter)) + if got := s.stalledStragglers(appsV3, t0.Add(stallAfter+time.Second)); len(got) != 0 { + t.Errorf("an app that transitioned must not be immediately stalled, got %v", got) + } + // But if it now sits still for stallAfter, it stalls. + if got := s.stalledStragglers(appsV3, t0.Add(2*stallAfter)); len(got) != 1 { + t.Errorf("after stallAfter of no change it must stall, got %v", got) + } +} + +// TestStallTracker_ForgetsVanishedApps: an app that disappears and later +// reappears starts a fresh clock rather than inheriting a stale one. +func TestStallTracker_ForgetsVanishedApps(t *testing.T) { + s := newStallTracker() + t0 := time.Unix(0, 0) + apps := []Application{{Name: "mongodb", Health: ArgoCDHealthHealthy, Sync: ArgoCDSyncOutOfSync}} + s.observe(apps, t0) + + // It vanishes from the listing. + s.observe(nil, t0.Add(30*time.Second)) + // It reappears much later — its clock must start now, not at t0. + s.observe(apps, t0.Add(10*time.Minute)) + if got := s.stalledStragglers(apps, t0.Add(10*time.Minute+stallAfter-time.Second)); len(got) != 0 { + t.Errorf("a reappearing app must start a fresh clock, got %v", got) } } diff --git a/internal/chart/providers/argocd/values_test.go b/internal/chart/providers/argocd/values_test.go index e1a95556..ae4a2e97 100644 --- a/internal/chart/providers/argocd/values_test.go +++ b/internal/chart/providers/argocd/values_test.go @@ -118,12 +118,13 @@ func TestArgoCDValues_ControllerExtraArgs(t *testing.T) { // scheduling on the small k3d clusters the CLI targets. func TestArgoCDValues_AllComponentsHaveResources(t *testing.T) { v := parseValues(t) + // dex is intentionally excluded: it is disabled (see TestArgoCDValues_DexDisabled), + // so it schedules no pod and needs no resource block. components := map[string]component{ "controller": v.Controller, "server": v.Server, "repoServer": v.RepoServer, "redis": v.Redis, - "dex": v.Dex, "applicationSet": v.ApplicationSet, "notifications": v.Notifications, } @@ -138,3 +139,25 @@ func TestArgoCDValues_AllComponentsHaveResources(t *testing.T) { } } } + +// TestArgoCDValues_DexDisabled locks the V1 blocker fix: the argo-cd chart +// defaults dex.enabled to true, and the dexidp/dex:v2.45.1 arm64 image +// intermittently SIGSEGVs under emulation on Apple Silicon, CrashLoopBackOff-ing +// the 7-minute `helm --wait` into a fresh-install failure. OpenFrame's login +// uses the local developer account, never dex, so it must be disabled. +func TestArgoCDValues_DexDisabled(t *testing.T) { + var raw struct { + Dex struct { + Enabled *bool `yaml:"enabled"` + } `yaml:"dex"` + } + if err := yaml.Unmarshal([]byte(GetArgoCDValues()), &raw); err != nil { + t.Fatal(err) + } + if raw.Dex.Enabled == nil { + t.Fatal("dex.enabled must be set explicitly (chart default is true); it is absent") + } + if *raw.Dex.Enabled { + t.Error("dex.enabled must be false — dex is unused and its arm64 image crashes on Apple Silicon") + } +} diff --git a/internal/chart/providers/argocd/wait.go b/internal/chart/providers/argocd/wait.go index 6f0c70f4..07565cda 100644 --- a/internal/chart/providers/argocd/wait.go +++ b/internal/chart/providers/argocd/wait.go @@ -179,9 +179,8 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn // Once an app is ready, it stays counted even if it temporarily goes out of sync everReadyApps := make(map[string]bool) - // Stall tracking (finding N3) — see stall.go. - lastStallFingerprint := "" - lastProgressAt := time.Now() + // Stall tracking (finding N3, per-application — see stall.go). + stall := newStallTracker() stragglerSyncTriggered := false stallHintShown := false @@ -367,32 +366,27 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn lastNotReadyApps, lastReadyCount, lastTotalApps = notReadyApps, currentlyReady, totalApps lastNotReadyNames = assess.notReadyNames - // Stall handling (finding N3): when the state has been bit-for-bit - // identical for stallAfter and OutOfSync-but-healthy stragglers - // remain, waiting further is futile for apps with autoSync off. + // Stall handling (finding N3, per-application): an app that has sat + // OutOfSync-but-Healthy, bit-for-bit identical, for stallAfter will not + // move on its own (autoSync off). Judged per-app so a noisy neighbour + // flapping status can't keep resetting the clock on a stuck app (V5). // On the upgrade path, sync exactly those stragglers once; otherwise // print an actionable hint once instead of burning the full timeout. - if fp := stallFingerprint(currentlyReady, notReadyApps); fp != lastStallFingerprint { - lastStallFingerprint = fp - lastProgressAt = time.Now() - } else if len(notReadyApps) > 0 && time.Since(lastProgressAt) > stallAfter { - if stragglers := outOfSyncStragglers(apps); len(stragglers) > 0 { - if config.SyncStragglersOnStall && !stragglerSyncTriggered { - stragglerSyncTriggered = true - pterm.Warning.Printf("No progress for %s; triggering sync of %d OutOfSync application(s): %v\n", - stallAfter.Round(time.Second), len(stragglers), stragglers) - patched, failedCount, syncErr := m.syncApplicationsByName(localCtx, stragglers, false) - if failedCount > 0 { - pterm.Warning.Printf("Straggler sync: %d triggered, %d failed (first error: %v)\n", patched, failedCount, syncErr) - } - lastProgressAt = time.Now() // give the sync room to act - } else if !stallHintShown { - stallHintShown = true - pterm.Warning.Printf("No progress for %s; %d application(s) are OutOfSync and may have auto-sync disabled: %v\n", - stallAfter.Round(time.Second), len(stragglers), stragglers) - pterm.Info.Println("They will not sync on their own — run `openframe app upgrade --sync` (or sync them in ArgoCD) to roll them out.") - lastProgressAt = time.Now() // print the hint once per stall, not every tick + stall.observe(apps, time.Now()) + if stragglers := stall.stalledStragglers(apps, time.Now()); len(stragglers) > 0 { + if config.SyncStragglersOnStall && !stragglerSyncTriggered { + stragglerSyncTriggered = true + pterm.Warning.Printf("No progress for %s; triggering sync of %d OutOfSync application(s): %v\n", + stallAfter.Round(time.Second), len(stragglers), stragglers) + patched, failedCount, syncErr := m.syncApplicationsByName(localCtx, stragglers, false) + if failedCount > 0 { + pterm.Warning.Printf("Straggler sync: %d triggered, %d failed (first error: %v)\n", patched, failedCount, syncErr) } + } else if !stallHintShown { + stallHintShown = true + pterm.Warning.Printf("No progress for %s; %d application(s) are OutOfSync and may have auto-sync disabled: %v\n", + stallAfter.Round(time.Second), len(stragglers), stragglers) + pterm.Info.Println("They will not sync on their own — run `openframe app upgrade --sync` (or sync them in ArgoCD) to roll them out.") } } @@ -545,6 +539,17 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn spinnerStopped = true } spinnerMutex.Unlock() + + // Everything is Healthy+Synced — but "ready" is not "correct". + // If a ref was requested, confirm ArgoCD is actually tracking it + // before declaring success; a legacy branch's chart silently + // deploys main and this is the only place that catches it (V3). + if config.AppOfApps != nil { + if mm := verifyRefPinning(apps, config.AppOfApps.GitHubRepo, config.AppOfApps.GitHubBranch); len(mm) > 0 { + return refMismatchError(config.AppOfApps.GitHubBranch, mm) + } + } + pterm.Success.Println("All ArgoCD applications installed") return nil } diff --git a/internal/chart/providers/helm/argocd_wait.go b/internal/chart/providers/helm/argocd_wait.go index 7d3c044b..6b69fa10 100644 --- a/internal/chart/providers/helm/argocd_wait.go +++ b/internal/chart/providers/helm/argocd_wait.go @@ -12,7 +12,6 @@ import ( "github.com/flamingo-stack/openframe-cli/internal/k8s" "github.com/flamingo-stack/openframe-cli/internal/platform" "github.com/flamingo-stack/openframe-cli/internal/shared/executor" - "github.com/flamingo-stack/openframe-cli/internal/shared/redact" "github.com/pterm/pterm" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -217,12 +216,11 @@ func (h *HelmManager) verifyHelmRelease(ctx context.Context, releaseName, namesp return fmt.Errorf("failed to run helm list: %w", err) } - // Log the helm list output. Redact at the print site (the struct value is - // parsed below): helm output may carry values echoed back from the release. - if verbose { - pterm.Info.Println("Helm list output:") - pterm.Println(redact.Redact(result.Stdout)) - } + // The raw `helm list` JSON is not printed even under --verbose (V6): it is an + // implementation detail of the existence check below, and dumping it (plus + // the full `helm status` inventory further down) buried the useful lines + // under 150+ lines of noise. The compact "verified successfully" line at the + // end carries the signal. // Check if the release exists in the output // The JSON output will be an empty array "[]" if no releases found @@ -231,27 +229,24 @@ func (h *HelmManager) verifyHelmRelease(ctx context.Context, releaseName, namesp return fmt.Errorf("helm release '%s' not found in namespace '%s' - helm list returned empty", releaseName, namespace) } - // Also run helm status for more details + // Also run `helm status` as a second existence/health probe. Only its exit + // status matters — a release that exists but whose status errors is a real + // problem. The command's multi-hundred-line resource inventory + NOTES are + // intentionally discarded, not printed even under --verbose (V6). statusArgs := []string{"status", releaseName, "-n", namespace} if clusterName != "" { contextName := k8s.ResolveContextForCluster(k8s.DefaultKubeconfigPath(), clusterName) statusArgs = append(statusArgs, "--kube-context", contextName) } - statusResult, err := h.executor.ExecuteWithOptions(ctx, executor.ExecuteOptions{ + if _, err = h.executor.ExecuteWithOptions(ctx, executor.ExecuteOptions{ Command: "helm", Args: statusArgs, Env: h.getHelmEnv(), - }) - if err != nil { + }); err != nil { return fmt.Errorf("helm release exists but status check failed: %w", err) } - if verbose { - pterm.Info.Println("Helm status output:") - pterm.Println(redact.Redact(statusResult.Stdout)) - } - pterm.Success.Printf("Helm release '%s' verified successfully\n", releaseName) return nil } diff --git a/internal/chart/providers/helm/manager.go b/internal/chart/providers/helm/manager.go index 303e8406..ac632fbf 100644 --- a/internal/chart/providers/helm/manager.go +++ b/internal/chart/providers/helm/manager.go @@ -406,20 +406,13 @@ func (h *HelmManager) InstallArgoCDWithProgress(ctx context.Context, config conf return fmt.Errorf("failed to install ArgoCD: %w", err) } - // Log Helm output for debugging (helps identify if Helm actually created - // resources). Stdout is redacted at the PRINT site (not in the result - // struct, which callers parse): helm's rendered output can echo values — - // including the docker registry password — in verbose/dry-run mode. - // Stderr arrives already redacted by the executor. - if config.Verbose && result != nil { - if result.Stdout != "" { - pterm.Info.Println("Helm stdout:") - pterm.Println(redact.Redact(result.Stdout)) - } - if result.Stderr != "" { - pterm.Info.Println("Helm stderr:") - pterm.Println(result.Stderr) - } + // On success, helm's stdout is just the release summary + chart NOTES (100+ + // lines) — pure noise that buried the useful --verbose output (V6), so it is + // not printed. Stderr, when present, carries deprecation/ownership warnings + // worth seeing; it arrives already redacted by the executor. + if config.Verbose && result != nil && result.Stderr != "" { + pterm.Info.Println("Helm stderr:") + pterm.Println(result.Stderr) } // Dry-run creates nothing: helm ran with --dry-run=client and the executor diff --git a/internal/shared/errors/friendly.go b/internal/shared/errors/friendly.go index 47e2f8b0..409fbaf5 100644 --- a/internal/shared/errors/friendly.go +++ b/internal/shared/errors/friendly.go @@ -20,6 +20,12 @@ func friendlyHint(err error) string { // often surfaces wrapped in a timeout-looking message). case containsAny(msg, "invalid ownership metadata", "cannot be imported into the current release", "missing key \"meta.helm.sh"): return "A resource (usually the ArgoCD CRDs) already exists without Helm ownership metadata. Recreate the cluster ('openframe cluster delete' + 'openframe cluster create'), or add the Helm ownership labels to that resource, then retry." + // A prior helm operation was interrupted and left the release wedged in a + // pending-* state. Matched BEFORE the generic timeout case: retrying hits the + // SAME pending release and fails identically, so the timeout hint ("wait and + // retry") is actively wrong here — the fix is a rollback. + case containsAny(msg, "another operation (install/upgrade/rollback) is in progress", "pending-install", "pending-upgrade", "pending-rollback"): + return "A previous install/upgrade was interrupted and left the release in a pending state; retrying will not clear it. Roll it back with 'helm rollback -n ' (or 'helm uninstall' it), then run the command again. Tip: pending releases are hidden from plain 'helm list' — use 'helm list -a'." case containsAny(msg, "connection refused", "was refused", "unable to connect to the server", "connection reset"): return "The cluster isn't reachable — is it running? Try 'openframe cluster status'." case containsAny(msg, "no such host", "dns resolution", "name resolution"): diff --git a/internal/shared/errors/friendly_upstream_test.go b/internal/shared/errors/friendly_upstream_test.go index e855be7e..bbbef61a 100644 --- a/internal/shared/errors/friendly_upstream_test.go +++ b/internal/shared/errors/friendly_upstream_test.go @@ -86,3 +86,24 @@ func TestFriendlyHint_NoHintForUnknownErrors(t *testing.T) { t.Errorf("expected no hint for an unrecognized error, got %q", got) } } + +// TestFriendlyHint_PendingReleaseSuggestsRollback (V4): a helm release left in +// pending-* by an interrupted operation must point at rollback, NOT the generic +// "wait and retry" timeout hint — retrying hits the same wedged release. The +// pending case is ordered before the timeout case for exactly this reason. +func TestFriendlyHint_PendingReleaseSuggestsRollback(t *testing.T) { + cases := []string{ + `Error: UPGRADE FAILED: another operation (install/upgrade/rollback) is in progress`, + `Error: release app-of-apps failed, status: pending-upgrade`, + `cannot patch: release in pending-install state`, + } + for _, msg := range cases { + got := friendlyHint(fmt.Errorf("op failed: %s", msg)) + if !strings.Contains(got, "rollback") { + t.Errorf("pending-release must suggest rollback.\nmessage: %s\ngot: %q", msg, got) + } + if strings.Contains(got, "cluster may be slow or unreachable") { + t.Errorf("pending-release must NOT get the generic timeout hint (retry is wrong here).\nmessage: %s\ngot: %q", msg, got) + } + } +} From 4c8af855b7411184ed996752d9251588d160f886 Mon Sep 17 00:00:00 2001 From: Oleg Tkachuk Date: Mon, 13 Jul 2026 13:21:24 +0300 Subject: [PATCH 02/11] feat(cli): heartbeat during the silent helm --wait installs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the ArgoCD (7m) and app-of-apps helm installs block on `helm --wait` with no output while they run. On a TTY the spinner animates, but in a non-interactive / CI / piped session there is no spinner, so the terminal sat silent for minutes between "Installing..." and the result — and the 0.4.8 verification found users assume a hang and kill the process before the diagnostics ever print, defeating the diagnostic work itself. Add a Heartbeat primitive (ui/spinner/heartbeat.go): the non-interactive counterpart to the animated Spinner. It emits "