fix(bundler): argocd-helm repoURL UX — surface key, emit install hint#1081
Merged
Conversation
Closes the two actionable observations from #1020 (the other two were closed during the issue's status refresh: "bake repoURL into chart" was wontfix-by-design — bundles are URL-portable per the publish-once / mirror-many constraint, and "fail at bundle time" is incompatible with that same design). ## values.yaml surfacing argocdhelm.Generate now writes the bundle's root values.yaml through a new writeRootValuesFile helper that prepends an "Install-time inputs" header explaining repoURL and targetRevision, and injects both keys with empty-string defaults. The parent App template's {{ required }} directive still enforces non-empty at render time — empty defaults only affect documentation surface (helm show values now lists the keys; before, they were invisible). The doc header wording reflects the post-#1051 native-OCI contract: repoURL is the PARENT namespace, no chart name, because the parent App template appends .Chart.Name itself. ## OCI install hint runBundleCmd now calls printArgoCDHelmOCIInstructions after a successful OCI push when --deployer argocd-helm. The helper formats: argocd-helm bundle pushed: oci://<reg>/<repo>:<tag> To install: helm install <release> oci://<reg>/<repo>:<tag> \ --namespace argocd \ --set repoURL=oci://<reg>/<parent-path> Parent-path derivation uses path.Dir(Repository); the helper degrades gracefully to a registry-only repoURL when Repository is a single segment (path.Dir → "."). Skips silently for non-OCI references. ## Scoped to argocd-helm intentionally Other deployers' OCI publishes don't ship a Helm chart — they're generic AICR artifacts with their own distribution path. Emitting a helm install line for them would be wrong; the existing --image-refs digest file is the canonical post-push artifact for those flows. ## Testing - New TestPrintArgoCDHelmOCIInstructions covers four shapes: nested namespace, deeply nested, single-segment (parent collapses to registry), nil/non-OCI (silent skip). Includes an assertion that the --set repoURL line never contains the chart name segment, to pin the post-#1051 contract against future refactors that might reintroduce double-append. - argocdhelm goldens regenerated via go test -args -update; both testdata/helm_and_manifest_only/values.yaml and testdata/mixed_component/values.yaml now ship with the new header + repoURL/targetRevision keys. - make qualify clean: lint, race, e2e chainsaw (22 tests), grype, license headers. Fixes #1020
|
Actionable comments posted: 0 |
|
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 (5)
📝 WalkthroughWalkthroughThis PR addresses issue Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (1 decrease, 1 increase)
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. |
lalitadithya
approved these changes
May 28, 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
Close the two actionable observations from #1020: surface
repoURLandtargetRevisionin the argocd-helm bundle's rootvalues.yaml(sohelm show valuesdocuments them), and print the canonicalhelm installline at the end of an OCI push so contributors don't have to derive it from the{{ required }}error message.Motivation / Context
#1020 originally listed four UX gaps on argocd-helm OCI publishes. The issue's status refresh (2026-05-28) closed two of them:
repoURLinto chart at build time" — wontfix-by-design. The bundle is URL-portable per the publish-once / mirror-many constraint (pkg/cli/bundle.go:175-184,argocdhelm.go:606-614). Baking the publish URL breaks that.The two remaining gaps — both documentation-shaped — are what this PR fixes.
Fixes: #1020
Type of Change
Component(s) Affected
pkg/bundler,pkg/component/*)cmd/aicr,pkg/cli)Implementation Notes
values.yamlsurfacing (pkg/bundler/deployer/argocdhelm/argocdhelm.go)argocdhelm.Generatenow writes the bundle's rootvalues.yamlvia a newwriteRootValuesFilehelper that:repoURLandtargetRevision, with example--setlines.helm show valueslists them.The parent App template's
{{ required }}directive atargocdhelm.go:479, 811still enforces non-empty at render time — the empty defaults only affect documentation surface, not runtime behavior. The header wording matches the post-#1051 native-OCI contract:repoURLis the parent namespace, no chart name, because the parent App template appends.Chart.Nameitself.OCI install hint (
pkg/cli/bundle.go)runBundleCmdnow calls a newprintArgoCDHelmOCIInstructionsafter a successful OCI push when--deployer argocd-helm. Output:Parent-path derivation uses
path.Dir(Repository); the helper degrades gracefully to a registry-onlyrepoURLwhenRepositoryis a single segment (path.Dir → "."). Skips silently for nil or non-OCI references.Scope deliberately limited to argocd-helm
Other deployers' OCI publishes don't ship a Helm chart — they're generic AICR artifacts with their own distribution path. Emitting a
helm installline for them would be wrong; the existing--image-refsdigest file is the canonical post-push artifact for those flows.Testing
New
TestPrintArgoCDHelmOCIInstructionscovers four shapes:oci://ghcr.io/nvidia/aicr-bundle:v1.0.0)registry.example.com/team/platform/aicr-bundle:0.42.0)repoURL)Includes an assertion that the
--set repoURL=line never contains the chart-name segment — pins the post-#1051 contract against future refactors that might reintroduce double-append.argocdhelm goldens were regenerated via
go test ./pkg/bundler/deployer/argocdhelm/... -run TestBundleGolden -args -update. Two golden values.yaml files (helm_and_manifest_only/,mixed_component/) now ship with the new header + the two surfaced keys.Coverage delta (
pkg/cli,pkg/bundler/deployer/argocdhelm): both packages pass the project-wide 70% floor; per-package coverage onpkg/cliimproved (newprintArgoCDHelmOCIInstructionsis fully exercised by the new test).Risk Assessment
requiredtemplate directive still gates render-time non-empty; nothing about the runtime contract changes. Existing bundles continue to install identically. New bundles ship a more verbosevalues.yaml(additional 14 lines of comments + 2 empty-string keys) and OCI publishes emit one block ofstdoutper push.Rollout notes: None. No migration. No flag flips. Consumers who scrape
values.yamlviahelm show valueswill start seeing two new keys and a documentation header — that's the intended outcome.Checklist
make testwith-race)make lint)--set repoURLsemantics atdocs/user/cli-reference.md:1592-1600; the contract didn't change)git commit -S)