fix(bundler): wire PreManifestFiles through flux deployer with terminal-aware dependsOn

[yuanchen8911]

Summary

Wire ComponentPreManifests through the flux deployer so the synthesized GKE critical-priority ResourceQuota from #921 (and any future PreManifestFiles consumer such as the os-talos mixin) actually reaches --deployer flux bundles. Also fix a pre-existing chain-ordering bug where the next component depended on the previous component's primary name instead of its terminal release.

Motivation / Context

PR #921 fixed #915 (GKE ResourceQuota admission blocks system-*-critical pods outside kube-system) for the helm, helmfile, argocd, and argocd-helm deployers by synthesizing a pre-phase ResourceQuota via collectComponentPreManifests. The PR description claimed flux benefits too, but flux.Generator never had a ComponentPreManifests field and the DeployerFlux switch case never called collectComponentPreManifests — so the synthesized quota was computed and silently dropped. A GKE bundle generated with --deployer flux still hit the original symptom.

The same gap also blocked the os-talos mixin (#856) from working on flux, since both consumers ride on ComponentRef.PreManifestFiles.

While addressing the issue, Codex review surfaced a second, pre-existing bug: every component's dependsOn resolved to the previous component's primary name, even when that previous component had -post manifests. Flux could reconcile the next component in parallel with the previous component's post manifests, defeating the chain's purpose. Codex verified this against a local GKE Flux bundle: gke-nccl-tcpxo.dependsOn = gpu-operator instead of gpu-operator-post.

Fixes: #923
Related: #915, #921, #856

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• Documentation update

Component(s) Affected

• Bundlers (pkg/bundler, pkg/component/*)
• Docs/examples (docs/, examples/)

Implementation Notes

• flux.Generator field — add ComponentPreManifests map[string]map[string][]byte, mirroring the field already present on helm, helmfile, argocd, and argocd-helm generators.
• Bundler wiring — bundler.buildDeployer's DeployerFlux case now calls collectComponentPreManifests alongside collectComponentManifests and threads both into the flux generator.
• <name>-pre emission — when ComponentPreManifests[ref.Name] is non-empty, generateComponentResources emits a <name>-pre HelmRelease via the existing generateManifestHelmChart helper (same path used for -post) and rewires the primary's dependsOn to <name>-pre. Chain becomes previous → <name>-pre → <name> → <name>-post → next.
• Terminal-aware dependsOn — replace free buildDependsOn(sortedRefs, index) with method g.buildPrimaryDependsOn, backed by a new free function terminalReleaseNameFor(ref, postManifests) that returns <name>-post for mixed components and <name> otherwise. The README renderer (buildComponentSummaries) uses the same helper, so the rendered README matches the actual chain.
• Collision guard — at Generate() entry, a recipe declaring both foo (with pre-manifests) and a separate foo-pre is rejected at bundle time. Mirrors the rule in pkg/bundler/deployer/localformat/writer.go.
• No localformat re-use — flux has its own template-driven path and doesn't go through localformat, so the synthesis lives directly in flux.go. Same applies to validatePreManifestNamespace; flux doesn't yet have an os-talos-shaped consumer, so the namespace-drift guard isn't ported. Can be added later if a flux user needs it.

Testing

make qualify   # all 21 chainsaw tests pass, including cli-bundle-flux

New / updated tests in pkg/bundler/deployer/flux/flux_test.go:

• TestBuildPrimaryDependsOn (4 sub-cases) — exercises terminal resolution for mixed, manifest-only, and chart-only previous components. Replaces TestBuildDependsOn since buildDependsOn is gone.
• TestGenerate_WithPreManifests — pre-only component emits <name>-pre/ with Chart.yaml + templates + HelmRelease, pre HelmRelease depends on previous component, primary depends on <name>-pre (not previous), root kustomization references the pre folder.
• TestGenerate_PreAndPostManifests — full pre + post + downstream-next shape. Asserts pre is head of chain (no dependsOn), primary depends on <name>-pre, post depends on <name>, and the next component depends on <name>-post (regression guard for Codex's finding).
• TestGenerate_PreManifestsCollision — recipe declaring both foo (with pre-manifests) and foo-pre is rejected with an error naming the offending pair.

Coverage: pkg/bundler/deployer/flux 83.5% → 83.9% (+0.4%).

Risk Assessment

• Low — Isolated to the flux deployer; non-GKE / non-pre-manifest recipes generate the same bytes as before (terminal-aware dependsOn only changes output for mixed previous components, which previously had the next release racing the post anyway — a latent correctness improvement, not a behavior change for the common case). Well-tested, easy to revert.

Rollout notes: For flux bundles where the previous component is mixed (chart + post-manifests), the next component's dependsOn will now resolve to <prev>-post rather than <prev>. This is the documented chain — operators tracking reconcile order should see the expected sequencing rather than parallel reconcile of the next component with the previous component's post manifests. No CLI flags, no API surface changes.

Checklist

• Tests pass locally (make test with -race)
• Linter passes (make lint)
• I did not skip/disable tests to make CI green
• I added/updated tests for new functionality
• I updated docs if user-facing behavior changed
• Changes follow existing patterns in the codebase
• Commits are cryptographically signed (git commit -S)

[github-actions]

🌿 Preview your docs: https://nvidia-preview-fix-flux-pre-manifests-923.docs.buildwithfern.com/aicr

[mchmarny]

Solid fix that closes the gap PR #921 left for the flux deployer, plus a real win on the pre-existing terminal-aware dependsOn bug that Codex caught. Test coverage for the main paths is thorough, docs are updated in sync, and the collision guard pattern is the right shape.

Two medium notes worth considering before merge: the -post half of the localformat-parity collision check is still missing in flux (latent pre-existing gap, but the new docstring makes the omission visible), and the vendored-chart + pre-manifests path is exercised by the code but not by tests. Plus one nit on error-wrapping inconsistency in bundler.go. None are blockers.

CI looks healthy — tests / E2E and analyze are still running but every completed check is green.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

pkg/bundler/deployer/flux/flux.go (2)

625-651: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Extract HelmRelease into a named constant.

This literal is repeated in the same helper and is already tripping goconst.

♻️ Suggested change

 const (
 	fileChart         = "Chart.yaml"
 	fileConfigMap     = "configmap-values.yaml"
 	fileHelmRelease   = "helmrelease.yaml"
 	fileKustomization = "kustomization.yaml"
 	fileReadme        = "README.md"
+	componentSummaryTypeHelmRelease = "HelmRelease"
 )

-				Type:         "HelmRelease",
+				Type:         componentSummaryTypeHelmRelease,
...
-			Type:         "HelmRelease",
+			Type:         componentSummaryTypeHelmRelease,
...
-				Type:         "HelmRelease",
+				Type:         componentSummaryTypeHelmRelease,

As per coding guidelines "Extract hardcoded resource names from templates as named constants to keep code and templates in sync".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/bundler/deployer/flux/flux.go` around lines 625 - 651, Multiple
occurrences of the literal "HelmRelease" are used when constructing
ComponentSummary entries (see the summaries append blocks around preName,
ref.Name, and ref.Name+"-post"); extract this hardcoded resource type into a
named constant (e.g., const componentTypeHelmRelease = "HelmRelease") and
replace all literal occurrences in the helper where ComponentSummary{ Type:
"HelmRelease", ... } is set (including the isMixed branch and the pre/post
blocks) to use the new constant to satisfy goconst and keep templates in sync.

---

643-651: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the actual post-release emission rule here.

The generator emits <name>-post for any non-manifest-only component with post-manifests, but this README path only adds that row when both ref.Chart and ref.Source are set. Git/path-backed components with post-manifests will therefore render an incomplete chain in the README.

🩹 Suggested fix

-		isMixed := ref.Chart != "" && ref.Source != "" && len(manifests[ref.Name]) > 0
-		if isMixed {
+		hasPostRelease := terminalReleaseNameFor(ref, manifests) == ref.Name+"-post"
+		if hasPostRelease {
 			summaries = append(summaries, ComponentSummary{
 				Name:         ref.Name + "-post",
 				Type:         "HelmRelease",
 				Namespace:    ref.Namespace,
 				DependsOnStr: ref.Name,
 			})
 		}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/bundler/deployer/flux/flux.go` around lines 643 - 651, The README
generation currently only emits a post-release row when both ref.Chart and
ref.Source are set; change the isMixed check used when building the
ComponentSummary (symbol: isMixed, used to append Name: ref.Name + "-post") to
detect any non-manifest-only component with post-manifests by testing that
manifests[ref.Name] has entries AND at least one of ref.Chart or ref.Source is
present (i.e., use logical OR instead of AND), so git/path-backed components
with post-manifests also produce the "<name>-post" HelmRelease summary.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/bundler/deployer/flux/flux_test.go`:
- Around line 1900-1906: The test currently checks the collision error by string
matching on err.Error(), which is brittle; change the assertion to unwrap and
inspect the structured error returned by g.Generate(ctx, outputDir) by using
errors.As into a *errors.StructuredError (or equivalent type) and assert se.Code
== errors.ErrCodeInvalidRequest (using errors.As/Is per project conventions);
after confirming the error code, keep the existing secondary check that the
error message contains the offending pair text (`would inject "foo"-pre`) to
ensure the message names the colliding pair. Ensure you reference the error
value returned by g.Generate and the StructuredError type and
ErrCodeInvalidRequest constant in your replacement assertions.

---

Outside diff comments:
In `@pkg/bundler/deployer/flux/flux.go`:
- Around line 625-651: Multiple occurrences of the literal "HelmRelease" are
used when constructing ComponentSummary entries (see the summaries append blocks
around preName, ref.Name, and ref.Name+"-post"); extract this hardcoded resource
type into a named constant (e.g., const componentTypeHelmRelease =
"HelmRelease") and replace all literal occurrences in the helper where
ComponentSummary{ Type: "HelmRelease", ... } is set (including the isMixed
branch and the pre/post blocks) to use the new constant to satisfy goconst and
keep templates in sync.
- Around line 643-651: The README generation currently only emits a post-release
row when both ref.Chart and ref.Source are set; change the isMixed check used
when building the ComponentSummary (symbol: isMixed, used to append Name:
ref.Name + "-post") to detect any non-manifest-only component with
post-manifests by testing that manifests[ref.Name] has entries AND at least one
of ref.Chart or ref.Source is present (i.e., use logical OR instead of AND), so
git/path-backed components with post-manifests also produce the "<name>-post"
HelmRelease summary.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: ddcc6c2c-1d58-48cc-9ce8-004aef851d6c

📥 Commits

Reviewing files that changed from the base of the PR and between 86363b9 and b8e9f50.

📒 Files selected for processing (5)

• docs/user/cli-reference.md
• pkg/bundler/bundler.go
• pkg/bundler/deployer/flux/doc.go
• pkg/bundler/deployer/flux/flux.go
• pkg/bundler/deployer/flux/flux_test.go

[yuanchen8911]

@mchmarny — all 6 inline comments addressed at 2693e411 and replied inline. Ready for another look when you have a minute.

#	Reviewer	File:line	Finding	Fix
1	CodeRabbit	cli-reference.md:1213	"HelmRelease/Kustomization CRs" — flux doesn't emit Kustomization CRs	Dropped "/Kustomization", added clarifying sentence about plain kustomization.yaml
2	CodeRabbit	flux.go:615	README missing <name>-pre row + wrong primary dependsOn	buildComponentSummaries now takes both manifest maps, emits -pre row, rewires primary. README assertions added to existing test.
3	Mark	flux.go:230	-pre collision rule landed but -post parity missing (latent localformat gap)	Added -post collision rule with same wording; TestGenerate_PostManifestsCollision covers it
4	Mark	flux.go:422	Vendored chart + pre-manifests path untested	New TestGenerate_VendoredChartWithPreManifests exercises VendorCharts: true with pre-manifest
5	Mark	bundler.go:418	Error-wrapping inconsistency within DeployerFlux arm	Switched post-manifests block to errors.PropagateOrWrap for symmetry
6	Mark	flux.go:555	Positive feedback on terminal-aware fix	Acknowledged; no code change needed

Bonus refactor: collision detection extracted into (*Generator).detectInjectedReleaseCollisions so Generate() stays under the funlen 70-statement cap. make qualify clean (21/21 chainsaw, no vulns, lint clean).

[yuanchen8911]

Adding a 7th item I missed in the table above — CodeRabbit also posted a nit on flux_test.go:1970 after my first force-push.

#	Reviewer	File:line	Finding	Fix
7	CodeRabbit	flux_test.go:1970	Collision tests asserted error type via strings.Contains(err.Error(), ...) rather than structured error code	Added stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) to both TestGenerate_PreManifestsCollision and TestGenerate_PostManifestsCollision ahead of the existing substring check (substring kept — it asserts which name collided). Used *StructuredError.Is form per .claude/CLAUDE.md rule, not the errors.As suggestion.

Branch is now at 7194d8fc. make qualify clean.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

pkg/bundler/deployer/flux/flux.go (2)

643-668: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Extract HelmRelease into a named constant.

This hunk repeats the same literal three times, and the current lint run is already flagging it. Pulling it into a constant keeps CI green and avoids resource-type strings drifting across the Flux renderer.

Suggested fix

 const (
 	fileChart         = "Chart.yaml"
 	fileConfigMap     = "configmap-values.yaml"
 	fileHelmRelease   = "helmrelease.yaml"
 	fileKustomization = "kustomization.yaml"
 	fileReadme        = "README.md"
+	resourceTypeHelmRelease = "HelmRelease"
 )

 			summaries = append(summaries, ComponentSummary{
 				Name:         preName,
-				Type:         "HelmRelease",
+				Type:         resourceTypeHelmRelease,
 				Namespace:    ref.Namespace,
 				DependsOnStr: previousTerminal,
 			})
@@
 		summaries = append(summaries, ComponentSummary{
 			Name:         ref.Name,
-			Type:         "HelmRelease",
+			Type:         resourceTypeHelmRelease,
 			Version:      version,
 			Namespace:    ref.Namespace,
 			DependsOnStr: primaryDependsOn,
 		})
@@
 			summaries = append(summaries, ComponentSummary{
 				Name:         ref.Name + "-post",
-				Type:         "HelmRelease",
+				Type:         resourceTypeHelmRelease,
 				Namespace:    ref.Namespace,
 				DependsOnStr: ref.Name,
 			})

As per coding guidelines, "Extract hardcoded resource names from templates as named constants to keep code and templates in sync."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/bundler/deployer/flux/flux.go` around lines 643 - 668, The literal
"HelmRelease" is repeated in the ComponentSummary construction (see occurrences
inside the summaries append blocks where ComponentSummary is used for preName,
ref.Name, and ref.Name+"-post"); extract that resource-type string into a named
constant (e.g., const ResourceTypeHelmRelease = "HelmRelease") and replace the
three hardcoded occurrences with the constant, ensuring you update any nearby
uses that expect the same literal (references: ComponentSummary struct
initializations, variables preName, primaryDependsOn, isMixed and the summaries
append calls).

---

660-663: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the same mixed-component predicate as generation.

Line 662 requires both ref.Chart and ref.Source, but the generator logic around this helper treats any chart-or-source component with post manifests as emitting <name>-post. That means the README can still omit the post release for source-only/chart-only components even though Flux generates it.

Suggested fix

-		isMixed := ref.Chart != "" && ref.Source != "" && len(manifests[ref.Name]) > 0
+		hasChartOrSource := ref.Chart != "" || ref.Source != ""
+		isMixed := hasChartOrSource && len(manifests[ref.Name]) > 0

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/bundler/deployer/flux/flux.go` around lines 660 - 663, The
mixed-component predicate is too strict: change the isMixed check in flux.go
from requiring both ref.Chart and ref.Source to match the generator logic that
treats any chart-or-source component with post manifests as mixed; i.e., set
isMixed when (ref.Chart != "" || ref.Source != "") && len(manifests[ref.Name]) >
0 so components with either Chart or Source plus manifests will emit the
"<name>-post" HelmRelease (update the isMixed variable and any checks that rely
on it).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@pkg/bundler/deployer/flux/flux.go`:
- Around line 643-668: The literal "HelmRelease" is repeated in the
ComponentSummary construction (see occurrences inside the summaries append
blocks where ComponentSummary is used for preName, ref.Name, and
ref.Name+"-post"); extract that resource-type string into a named constant
(e.g., const ResourceTypeHelmRelease = "HelmRelease") and replace the three
hardcoded occurrences with the constant, ensuring you update any nearby uses
that expect the same literal (references: ComponentSummary struct
initializations, variables preName, primaryDependsOn, isMixed and the summaries
append calls).
- Around line 660-663: The mixed-component predicate is too strict: change the
isMixed check in flux.go from requiring both ref.Chart and ref.Source to match
the generator logic that treats any chart-or-source component with post
manifests as mixed; i.e., set isMixed when (ref.Chart != "" || ref.Source != "")
&& len(manifests[ref.Name]) > 0 so components with either Chart or Source plus
manifests will emit the "<name>-post" HelmRelease (update the isMixed variable
and any checks that rely on it).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 4d8e1736-166d-40e6-aa53-dd7b52ec6385

📥 Commits

Reviewing files that changed from the base of the PR and between 2693e41 and 7194d8f.

📒 Files selected for processing (5)

• docs/user/cli-reference.md
• pkg/bundler/bundler.go
• pkg/bundler/deployer/flux/doc.go
• pkg/bundler/deployer/flux/flux.go
• pkg/bundler/deployer/flux/flux_test.go