feat(bundler): add --dynamic flag for install-time values (#515)

[lockwobr]

Summary

Closes #515

• Add --dynamic component:path flag to aicr bundle that declares value paths as install-time parameters
• Helm deployer: splits into values.yaml (static) + cluster-values.yaml (dynamic stubs), deploy.sh passes both
• ArgoCD deployer: auto-detects --dynamic and generates a Helm chart app-of-apps instead of flat manifests — static values baked
  as chart files, dynamic values in root values.yaml, merged at render time via mustMergeOverwrite
• Introduces deployer.Deployer interface for mockability
• New argocdhelm deployer delegates to existing ArgoCD deployer then transforms output (no logic duplication)
• Validates --dynamic component keys against the registry (catches typos)
• Fully backwards compatible — no --dynamic = identical output

Test plan

• make qualify passes (unit tests, lint, e2e, scan)
• make e2e — 19 tests pass including new bundle-dynamic (12 steps)
• Helm smoke: aicr bundle -r recipe.yaml --dynamic gpuoperator:driver.version -o /tmp/test
• ArgoCD smoke: aicr bundle -r recipe.yaml --deployer argocd --dynamic gpuoperator:driver.version -o /tmp/test && helm template
  test /tmp/test
• Backwards compat: aicr bundle -r recipe.yaml -o /tmp/test (no cluster-values.yaml, no Chart.yaml)

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Refactoring (no functional changes)
• Build/CI/tooling

Component(s) Affected

• CLI (cmd/aicr, pkg/cli)
• API server (cmd/aicrd, pkg/api, pkg/server)
• Recipe engine / data (pkg/recipe)
• Bundlers (pkg/bundler, pkg/component/*)
• Collectors / snapshotter (pkg/collector, pkg/snapshotter)
• Validator (pkg/validator)
• Core libraries (pkg/errors, pkg/k8s)
• Docs/examples (docs/, examples/)
• Other: ____________

Implementation Notes

Testing

# Commands run (prefer `make qualify` for non-trivial changes)
make qualify

Risk Assessment

• Low — Isolated change, well-tested, easy to revert
• Medium — Touches multiple components or has broader impact
• High — Breaking change, affects critical paths, or complex rollout

Rollout notes:

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) — GPG signing info

[github-actions]

Coverage Report ✅

Metric	Value
Coverage	74.5%
Threshold	70%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-74.5%25-green)

Merging this branch changes the coverage (3 decrease, 3 increase)

Impacted Packages	Coverage Δ	🤖
github.com/NVIDIA/aicr/pkg/bundler	61.80% (-0.40%)	👎
github.com/NVIDIA/aicr/pkg/bundler/config	96.53% (-0.69%)	👎
github.com/NVIDIA/aicr/pkg/bundler/deployer	0.00% (ø)
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm	71.48% (+71.48%)	🌟
github.com/NVIDIA/aicr/pkg/bundler/deployer/helm	86.98% (+0.54%)	👍
github.com/NVIDIA/aicr/pkg/cli	35.57% (-0.21%)	👎
github.com/NVIDIA/aicr/pkg/component	77.65% (+1.96%)	👍

---

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/NVIDIA/aicr/pkg/bundler/bundler.go	58.20% (-0.16%)	378 (+37)	220 (+21)	158 (+16)	👎
github.com/NVIDIA/aicr/pkg/bundler/config/config.go	96.53% (-0.69%)	173 (+29)	167 (+27)	6 (+2)	👎
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm/argocdhelm.go	71.48% (+71.48%)	263 (+263)	188 (+188)	75 (+75)	🌟
github.com/NVIDIA/aicr/pkg/bundler/deployer/deployer.go	0.00% (ø)	0	0	0
github.com/NVIDIA/aicr/pkg/bundler/deployer/helm/helm.go	86.98% (+0.54%)	192 (+15)	167 (+14)	25 (+1)	👍
github.com/NVIDIA/aicr/pkg/cli/bundle.go	0.68% (-0.02%)	147 (+4)	1	146 (+4)	👎
github.com/NVIDIA/aicr/pkg/component/helpers.go	95.74% (+1.63%)	47 (+13)	45 (+13)	2	👍
github.com/NVIDIA/aicr/pkg/component/overrides.go	96.10% (+0.61%)	282 (+38)	271 (+38)	11	👍

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

[mchmarny]

Inline comments on the regex-based YAML transformation, repeated registry loading, and missing security test cases. Core design looks good — these are mainly about resilience and completeness.

[github-actions]

🌿 Preview your docs: https://nvidia-preview-feat-dynamic-bundle-values-515.docs.buildwithfern.com

[yuanchen8911]

Confirmed Issues

1. Critical: pkg/bundler/deployer/argocdhelm/argocdhelm.go#L329 and pkg/bundler/deployer/argocdhelm/argocdhelm.go#L391-L404 serialize the valuesObject Helm template through yaml.Marshal. That emits valuesObject: |- ..., so after helm template ArgoCD receives a string scalar instead of the object it expects. This will break reconciliation for generated argocd-helm Applications. The fix needs to avoid marshaling the raw template expression as a normal YAML string.
2. Major: pkg/bundler/bundler.go#L373-L425 returns directly from the new makeArgoCDHelmChart path without the post-generation steps used by the other deployers. In particular, it skips copyDataFiles and attestBundle, while the CLI still accepts and wires through --attest in pkg/cli/bundle.go#L427-L443. That means --data is ignored and --attest silently produces an unattested bundle for --deployer argocd-helm. This path should either run the shared post-processing or reject unsupported flags explicitly.
3. Major: pkg/bundler/deployer/helm/helm.go#L297-L312 passes the original input.ComponentValues[comp.Name] map into pkg/bundler/deployer/helm/helm.go#L543-L552, which removes dynamic paths in place via RemoveValueByPath. Unlike the new argocdhelm implementation, this path does not deep-copy first, so it mutates caller-owned state and can leak modified values to any caller that reuses the map after Make().

Two issues (1 and 2) are still unresolved on #527.

1. argocd-helm still generates helm.valuesObject incorrectly. The current code rewrites it as valuesObject: |- ..., which is still a YAML string scalar, not the object ArgoCD expects. The correct fix is to stop using valuesObject here and emit helm.values instead, since that field is meant to hold rendered YAML text.

2. argocd-helm still mishandles --data. makeArgoCDHelmChart now rejects --attest, but it still skips the shared post-processing path, so external --data files are silently dropped. The minimal fix is to reject --data explicitly for --deployer argocd-helm, matching the --attest behavior. The broader fix would be to add real --data support by copying data files into the bundle and including them in checksums.

[yuanchen8911]

Recommended Fixes

1. Fix argocd-helm valuesObject generation

The current fixValuesObjectTemplate() change is still wrong because it emits:

helm:
  valuesObject: |-
    ...

That is still a YAML string scalar, not the object ArgoCD expects for valuesObject.

Recommended fix

Use spec.source.helm.values instead of valuesObject.

values is a string field, which is exactly what this deployer is generating: Helm-templated YAML text that ArgoCD will consume as values.

Code changes

In pkg/bundler/deployer/argocdhelm/argocdhelm.go:

1. Rename convertToSingleSourceWithValuesObject to something like convertToSingleSourceWithValues.
2. Rename valuesObjectTmpl to valuesTmpl.
3. Change the generated helm block from:

"helm": map[string]any{
    "valuesObject": valuesObjectTmpl,
},

to:

"helm": map[string]any{
    "values": valuesTmpl,
},

4. Keep the merge template itself, e.g.:

valuesTmpl := fmt.Sprintf(
    `{{- $static := (.Files.Get "static/%s.yaml") | fromYaml | default dict -}}`+"\n"+
        `{{- $dynamic := index .Values %q | default dict -}}`+"\n"+
        `{{- mustMergeOverwrite $static $dynamic | toYaml | nindent 8 }}`,
    componentName, overrideKey,
)

5. Rename fixValuesObjectTemplate() to fixValuesTemplate() and make it rewrite:

      values: |-

instead of:

      valuesObject: |-

That means:

• change the lookup from helm["valuesObject"] to helm["values"]
• change the strings.Index(...) marker from " valuesObject:" to " values:"
• change the raw block header from " valuesObject: |-\n" to " values: |-\n"

Why this is correct

After Helm renders the chart template, the generated ArgoCD Application will contain:

spec:
  source:
    helm:
      values: |-
        driver:
          version: "580"

That matches ArgoCD’s API: values is a string, and the string contains YAML values.

Tests to update

In pkg/bundler/deployer/argocdhelm/argocdhelm_test.go:

• Rename TestFixValuesObjectTemplate to TestFixValuesTemplate
• Assert:
  • output contains values: |-
  • output does not contain valuesObject:
  • output still contains the raw Helm template lines

In the existing “transformed template uses valuesObject” test:

• rename it to “transformed template uses values”
• assert:
  • template contains values:
  • template does not contain valuesObject

2. Fix --data handling for --deployer argocd-helm

Right now makeArgoCDHelmChart() rejects --attest, but still silently drops external --data.

For this PR, the smallest correct fix is to reject --data explicitly, same as --attest.

Code changes

In pkg/bundler/bundler.go, inside makeArgoCDHelmChart():

1. Keep the existing --attest rejection.
2. Add a --data rejection using the current global data provider.

Example shape:

provider := recipe.GetDataProvider()
if layered, ok := provider.(*recipe.LayeredDataProvider); ok && len(layered.ExternalFiles()) > 0 {
    return nil, errors.New(errors.ErrCodeInvalidRequest,
        "--data is not yet supported with --deployer argocd-helm")
}

That should live near the existing:

if b.Config.Attest() {
    return nil, errors.New(errors.ErrCodeInvalidRequest,
        "--attest is not yet supported with --deployer argocd-helm")
}

Why this is correct

• It avoids silently dropping user input
• It matches the current --attest behavior
• It keeps the PR scope small
• Real --data support can be added later as a follow-up

Test to add

In pkg/bundler/bundler_test.go, add something like:

TestMake_ArgoCDHelmRejectsData

Test shape:

1. Create a temp external data dir
2. Write a minimal registry.yaml into it
3. Create a layered provider with recipe.NewLayeredDataProvider(...)
4. Set it with recipe.SetDataProvider(...)
5. Run bundler with config.WithDeployer(config.DeployerArgoCDHelm)
6. Assert:
   • Make(...) returns an error
   • the error mentions --data
   • the error mentions not yet supported

Also restore the original global data provider in defer so the test is isolated.

Recommended scope for this PR

• Fix issue 1 by switching to helm.values
• Fix issue 2 by explicitly rejecting --data

That gets the PR to a correct, internally consistent state without expanding scope into full external-data support for argocd-helm.

[yuanchen8911]

there are still unresolved issues. Please take a look.

#527 (comment)

[yuanchen8911]

LGTM. Both previously confirmed issues are fixed:

1. valuesObject -> values: Switched to helm.values (string field), matching ArgoCD's API. Tests verify valuesObject key is absent.
2. --data rejection: makeArgoCDHelmChart now explicitly rejects --data with a clear error, matching the --attest pattern.
3. Map mutation (prior round): DeepCopyMap correctly isolates mutations. manifest.Render uses the original map, not the mutated copy.

---

Non-blocking cosmetic nits (can be cleaned up in this PR or a follow-up):

• Duplicate comment at bundler.go#L375-L376: // Reject flags not yet supported by the argocd-helm deployer. appears twice
• Stale test names in argocdhelm_test.go#L289 and #L400: TestConvertToSingleSourceWithValuesObject and TestFixValuesObjectTemplate still reference "ValuesObject" despite the rename to "values"
• Stale package doc at argocdhelm.go#L38: says "when --deployer argocd AND --dynamic" but routing uses DeployerArgoCDHelm as a distinct deployer

[yuanchen8911]

/lgtm