fix(bundler): pin OCI sync for argocd-helm mixed-component -pre/-post children#1039
Conversation
… children Issue #1018 reported that `--deployer argocd-helm` bundles published to OCI registries fail to sync on the synthetic `<component>-pre` / `<component>-post` child Applications. The structural fix landed in #1035 (path-based child template now appends `/{{ .Chart.Name }}` to the rendered `source.repoURL` so Argo CD's generic OCI source resolves to the actual artifact). However, two stale doc references still pointed operators at the pre-#1035 contract — installing per those examples re-produces the original failure with a double-suffixed reference. Changes: 1. `pkg/bundler/deployer/argocdhelm/argocdhelm.go` — `writeParentApplicationTemplate` docstring example updated to pass `--set repoURL=<parent namespace>` (no chart name). The docstring is the canonical reference for the deployer contract; the bundle's emitted README, the parent App's `required` directive, and the path-based child's `required` directive all already use this form. 2. `docs/user/cli-reference.md` — the argocd-helm "Recommended deploy flow" example also still showed `--set repoURL=oci://<reg>/<path>/ aicr-bundle`. Replaced with the parent-namespace form, added the contract rationale inline, and added a new troubleshooting entry covering the three common failure modes for OCI sync (double-suffixed repoURL, Argo CD < v2.13, missing tag). 3. `pkg/bundler/deployer/argocdhelm/argocdhelm_test.go` — new `TestHelmTemplate_MixedComponentPreChildResolvesFromOCI` reproduces the exact recipe shape from #1018 (Helm component + sibling ComponentPreManifests producing an NNN-<name>-pre folder), runs `helm template --set repoURL=oci://<parent-namespace>`, and asserts the rendered `gpu-operator-pre` Application's `source.repoURL` equals `<parent-namespace>/<chart-name>`. Pins the post-#1035 contract for the mixed-component synthetic-split code path, which `TestHelmTemplate_RendersWithSetRepoURL` only covered for manifest-only components. Fixes #1018 Related #1034, #1035
Coverage Report ✅
Coverage BadgeCoverage unchanged by this PR. |
|
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 (1)
📝 WalkthroughWalkthroughThis PR clarifies and validates the correct Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/user/cli-reference.md`:
- Around line 1609-1610: The sentence "If the cluster runs Argo CD < v2.13 *or*
the recipe is pure-Helm..." is misleading; change the wording so the version
clause is not presented as a standalone exception—make it clear that the
path-based children exception applies only to pure-Helm recipes
(manifest-only/mixed components require Argo CD >= v2.13). Update the line
containing that exact phrase and preserve the troubleshooting pointer about
`Failed to load target state` for `aicr-stack` and `<component>-pre`/`-post`
Applications, e.g., rephrase to say path-based children are not exercised for
pure-Helm recipes (otherwise Argo CD v2.13+ is required).
🪄 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: ea8eef09-fb1a-42cb-ad18-d62d095553f8
📒 Files selected for processing (3)
docs/user/cli-reference.mdpkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.go
The previous "if Argo < v2.13 *or* pure-Helm" phrasing implied that an older Argo install alone exempted the bundle from the v2.13 requirement, which is backwards — path-based children are always emitted when the recipe has manifest-only / mixed components; older Argo just fails to resolve them. Split into two sentences per CodeRabbit's suggestion: the pure-Helm recipe is the actual exception that lets older Argo work; otherwise v2.13+ is required.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
LGTM — clean fix for #1018. Regression test is well-targeted (synthetic-split path + parent + sibling upstream-helm child assertions), and the docs add useful Argo CD ≥ v2.13 prereq and troubleshooting context.
Non-blocking issue (minor):
tests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml#L206 still passes --set repoURL=oci://example/aicr-bundle, which is the old chart-qualified form. With the default chart name aicr-bundle, path-based children render as oci://example/aicr-bundle/aicr-bundle:v1. The test still passes because the assertion at L220 is a substring grep. Please change it to --set repoURL=oci://example so the test exercises the contract this PR documents.
Cross-review by Claude Code + Codex + CodeRabbit.
2 participants
Summary
Closes #1018 by (a) regression-testing the post-#1035 contract on the mixed-component
-pre/-postsynthetic-split code path that was the actual failure mode in the bug report, and (b) correcting two stale documentation examples that still pointed operators at the pre-#1035 contract.Motivation / Context
#1018 reported that
--deployer argocd-helmbundles published to OCI fail to sync on the<component>-pre/<component>-postApplications (the bug example wasgpu-operator-pre). The structural fix landed in #1035 — the path-based child template now appends/{{ .Chart.Name }}to the renderedsource.repoURLso Argo CD's generic OCI source resolves to the actual artifact when the operator passes--set repoURL=<parent namespace>.But two doc references were missed in #1035's sweep and still showed
--set repoURL=oci://<your-registry>/<path>/aicr-bundle(with chart segment). An operator following those examples gets a double-suffixed reference (.../aicr-bundle/aicr-bundle:<tag>), which Argo cannot resolve — reproducing #1018's exact failure. Without a regression test specifically exercising the synthetic-split path, the symptom can come back.Fixes: #1018
Related: #1034, #1035
Type of Change
Component(s) Affected
pkg/bundler,pkg/component/*)docs/,examples/)Implementation Notes
Code doc fix —
pkg/bundler/deployer/argocdhelm/argocdhelm.go. ThewriteParentApplicationTemplatedocstring example now uses--set repoURL=oci://ghcr.io/myorg(parent namespace). It also references #1018 / #1034 so anyone reading the source while debugging an OCI-sync issue lands on the right context.User doc fix —
docs/user/cli-reference.md. The argocd-helm "Recommended deploy flow" example now matches the post-#1032/#1035 contract, with a short inline rationale ("the parent App appends.Chart.Name; including the chart name in--set repoURLdouble-suffixes both the parent and the path-based children"). Added a new troubleshooting entry under "Bundle Generation Fails" enumerating the three common OCI-sync failure modes (double-suffixed repoURL, Argo CD < v2.13, missing tag) with the diagnostic commands for each.Regression test —
TestHelmTemplate_MixedComponentPreChildResolvesFromOCIinpkg/bundler/deployer/argocdhelm/argocdhelm_test.go. Reproduces the recipe shape from #1018 (gpu-operatorComponentRef + siblingComponentPreManifests), runs livehelm template --set repoURL=oci://example.test/myorg --set targetRevision=v9.9.9-issue-1018, and asserts:aicr-stackApplication:source.repoURL == oci://example.test/myorg,source.chart == aicr-bundle.gpu-operator-prechild:source.repoURL == oci://example.test/myorg/aicr-bundle,source.path == 001-gpu-operator-pre.gpu-operatorchild:source.repoURL == https://helm.ngc.nvidia.com/nvidia(must stay as the upstream registry, not get templated from.Values.repoURL).TestHelmTemplate_RendersWithSetRepoURLalready covered the manifest-only path-based shape (nodewright-customizations); this new test covers the synthetic-split shape thatComponentPreManifests/ComponentPostManifestsgenerate — the exact code path #1018 reported a failure on.Testing
make qualify # tests + lint + e2e (22 chainsaw cases) + vuln scan + license headers — all passedNew test runs in ~0.4s.
Risk Assessment
requireddirectives, which already enforced the parent-namespace form); regression test pins the post-fix(bundler): fix bugs - assemble full OCI artifact in path-based child apps; quote chart name (#1034) #1035 contract for the mixed-component path without changing any production code paths.Rollout notes: No migration. Operators already on the post-#1035 contract continue to work. Operators following the pre-#1035 example in the docs will, after this PR, see the corrected form and won't double-suffix their
--set repoURL.Checklist
make testwith-race)make lint)git commit -S)