fix(bundler): stamp AICR and payload versions into generated wrappers - #2571
Merged
Conversation
Generated wrapper charts hardcoded version 0.1.0, so every Kustomize-derived, manifest-only, and injected -pre/-post/-readiness release reported the same fictional version for both the artifact and its payload. Per ADR-021 Decision 7, version: now carries the AICR build that produced the wrapper, while appVersion and the new aicr.run/component-version annotation carry the payload version. Non-SemVer build versions fold to 0.0.0-dev so an unstamped build still produces a chart Helm will load. Signed-off-by: Mark Chmarny <mark@chmarny.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-fix-wrapper-chart-versions.docs.buildwithfern.com/aicr |
Contributor
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (41)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe bundlers now stamp generated Helm charts with a normalized AICR version and a separate payload version. Local, vendored, Flux, Argo CD, Helm, and Helmfile generation paths pass this metadata through chart templates. Development builds use Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to Generated wrapper metadata consistently separates Helm-valid AICR versions from payload versions across supported deployers and tests. No actionable merge risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Coverage Report ✅
Coverage BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
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
marked this pull request as ready for review
September 4, 2026 10:20
mchmarny
enabled auto-merge (squash)
September 4, 2026 10:21
lalitadithya
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Generated wrapper charts hardcoded
version: 0.1.0, so every Kustomize-derived component, manifest-only component, and injected-pre/-post/-readinessrelease reported the same fictional version for both the artifact and its payload. Per ADR-021 Decision 7,version:now carries the AICR build that produced the wrapper whileappVersionand a newaicr.run/component-versionannotation carry the payload version.Motivation / Context
helm list -Aenumerates every component of an AICR-deployed stack by name but reports0.1.0for the generated ones — the real payload version existed only inside a vendored wrapper'sdependencies:entry, and nowhere at all for the rest. The singleversion:field was being asked two questions at once and answering neither.Fixes: #2526
Related: #2424
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s) —pkg/defaultsdocs/,examples/)Implementation Notes
Both questions get their own field. A generated
Chart.yamlnow emits:localformat.stampForderives the payload version per folder: the upstream chart pin for Helm components, the git ref for Kustomize ones, and — for a component with no upstream pin (manifest-only, and the injected releases belonging to one) — AICR's own version, since that content is AICR's. Injected-pre/-post/-readinesswrappers inherit their parent's pin, so agpu-operator-postrelease reportsv25.3.0. The annotation is therefore always populated, and a reader never has to distinguish "absent" from "empty".The payload version is deliberately not normalized — a Kustomize ref like
release-1.4must not have to masquerade as SemVer, and matching the recipe pin byte-for-byte is what makes it comparable to one.The dev-build fold is load-bearing, and I confirmed the failure it prevents.
NormalizeChartVersionfolds any non-SemVer build version todefaults.DevChartVersion(0.0.0-dev). Against a realhelmbinary:The parse is the lenient
semver.NewVersion— the same call Helm makes when loading a chart — rather thanStrictNewVersion, so anything the helper returns will load.Scope beyond the issue's two files, and why. The issue names the two
localformattemplates. Two adjacent charts have the same defect and would otherwise be left reporting0.1.0(or failing to load) on a subset of deployers:flux/templates/chart.yaml.tmpl) also hardcoded0.1.0. Without it,--deployer fluxbundles would be the one place the acceptance criterion did not hold.NormalizeVersionWithDefault, which passesdevstraight through. I verified onmainthataicr bundle --deployer argocd-helmfrom a dev build emitsversion: "dev"and that the README's ownhelm install aicr-bundle .then fails to load the chart. One-line switch toNormalizeChartVersion; a regression case is added toTestGenerate_ChartVersion.The argocd-helm root chart deliberately does not join this contract. Its
version:comes from the recipe'smetadata.version— the AICR build that generated the recipe — while the wrappers inside the same bundle are stamped from the build that ranbundle. Those differ on the ordinary two-step workflow (generate a recipe once, bundle it later with a newer CLI), so oneargocd-helmbundle can show two AICR versions. I left the behavior alone rather than unifying it: that field also defaults the Argo CDtargetRevisionand the OCI push tag, so repointing it is a user-visible change outside this issue. The root chart carries neither annotation, so nothing reads it as the wrapper-provenance signal. Both the docs andchartVersion()'s comment now state the distinction explicitly.No deployer path read the wrapper
version:before this repurposed it (the issue's last acceptance bullet). Confirmed for all five: helmfile setsRelease.Versiononly forKindUpstreamHelmfolders (f.Upstream.Version) and emits local charts aschart: ./NNN-dirwith no version; Argo CD path-based Applications use the gittargetRevision; FluxHelmReleases for generated charts leaveHelmReleaseData.Versionempty and referencechart: ./<name>;install-local-helm.shinstalls./with no--version; and the only code that parses aChart.yamlversion (pkg/oci, for the OCI push tag) reads the argocd-helm root chart, not a wrapper.Annotation keys derive from
header.Domainper ADR-013 rather than being free-standing literals. The templates spell the keys out, because a templated key reads worse than the value it labels;TestAnnotationKeysMatchTemplatesis what keeps the two in sync, and fails loudly if a domain migration updates only one side.Quoting.
q(astrconv.Quotetemplate func, now on the shareddeployer.TemplateFuncs) emits YAML double-quoted scalars. This is not cosmetic: an unquotedappVersion: 1.2parses as a float and fails to unmarshal into Helm's string field. Same reasoning as the%qquoting already in the argocd-helmChart.yamlwriter (#1034).The
0.0.0-devfold is lossy, so it logs. Any non-SemVer build version folds to the same placeholder, which means a corrupted or hand-editedmetadata.versionbecomes indistinguishable from a genuine dev build. Erroring would block bundling over cosmetic metadata, soNormalizeChartVersionemitsslog.Warnfor the unexpected shapes while staying quiet for the two documented sentinels (""anddev).Reproducibility is unaffected in the sense the issue describes — the AICR version already travels in every bundle via
recipe.yaml. Worth noting explicitly:helmfiledoes not emitrecipe.yaml, so for that deployer, bundles containing generated wrappers now vary by AICR version where they previously did not.TestStockRenderParityGoldenpins the builder and bundler versions, so the golden digest stays a pure function of the catalog and the render.Testing
make qualify # exit 0 on the final treeNew coverage:
TestNormalizeChartVersion— table over release tags, pre-releases, build metadata,dev, empty, and branch-style refs. Every case additionally asserts the result parses as SemVer, so the invariant holds independently of the expectation.TestStampFor— payload derivation per component shape.TestWrite_StampsGeneratedWrappers— all five folder shapes in oneWrite, asserting the parsedChart.yamlfields.TestWrite_DevBuildProducesHelmValidCharts— walks every emittedChart.yamland fails on any non-SemVerversion:.TestAnnotationKeysMatchTemplates— template/constant sync guard across bothlocalformattemplates.TestRenderWrapperChartYAML_QuotesHostileVersion— a payload version carrying"and a newline must not escape its scalar, and must round-trip verbatim intoappVersionand the annotation.TestGenerate_StampsGeneratedCharts/_StampsVendoredWrapper/_DevBuildProducesHelmValidCharts(flux).TestGenerate_ChartVersiongains the unstamped-dev case (argocd-helm).manifest_onlygolden now pins001-node-prep/Chart.yaml, which was the one deployer whose wiring no golden covered.Each guard was mutation-tested: reverting
NormalizeChartVersionto a pass-through, droppingAICRVersionfromhelmfile'slocalformat.Options, and renaming the annotation key in a template each produce the expected failure.Beyond the unit tests, I generated real bundles from a
devbuild and ran the localhelmbinary against them:helm lintandhelm templateboth succeed on the generatedhelmwrapper and on theargocd-helmroot chart, where the latter fails onmain.Coverage (per-package, vs
origin/main):pkg/bundler/deployerpkg/bundler/deployer/localformatpkg/bundler/deployer/fluxpkg/bundler/deployer/{argocd,argocdhelm,helm,helmfile}pkg/defaultsADR-021's
## Statusnow records that Decision 7 shipped and marks step 2 of its Implementation Plan, so a contributor scoping the remaining decisions is not misled. Its problem statements are left as written at proposal time — they describe the state the ADR reasoned about, not the state ofmain.Risk Assessment
Every deployer's generated-chart output changes, so bundle bytes and
checksums.txtmove for any recipe with a generated wrapper. Rendered Kubernetes manifests are byte-identical, so there is no resource churn on redeploy.Rollout notes: One consequence the issue calls out and accepts: with
version:tracking the AICR version, a release that changes nothing material still shows a chart-version bump inhelm diff. Since rendered manifests are unchanged, that surfaces as a metadata-only diff, not a resource replacement. No migration steps; the annotations are additive and nothing reads them yet —upgrade-checkand the rest of #2424 remain post-v1.Checklist
make testwith-race)make lint)git commit -S)