feat(bundler): bake repoURL default into values.yaml at bundle push t…#1562
feat(bundler): bake repoURL default into values.yaml at bundle push t…#1562mohityadav8 wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds OCI parent-namespace plumbing from OCI bundle output parsing through bundler configuration into argocd-helm generation. For OCI outputs, the CLI now derives the parent namespace from the parsed reference and passes it into bundler config. The bundler config exposes that value via a new field, getter, and option. The argocd-helm generator uses it to default root Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: 6
🤖 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/bundler.go`:
- Line 405: The struct literal in the bundler configuration has a formatting
issue: the OCIParentNamespace field is missing the standard space/alignment used
by the other fields, so it will fail gofmt and lint. Update the composite
literal in bundler.go near the config construction so OCIParentNamespace matches
the alignment style used by fields like AppName and the rest of the b.Config
assignments.
In `@pkg/bundler/config/config.go`:
- Around line 468-472: The function bodies for OCIParentNamespace and
WithOCIParentNamespace are indented with spaces instead of tabs, which breaks
gofmt consistency. Update these methods in config.go to match the surrounding
style used by OCISourceName and the rest of the file, keeping the same logic but
using standard gofmt indentation throughout.
In `@pkg/bundler/deployer/argocdhelm/argocdhelm_test.go`:
- Around line 990-1036: Add coverage in TestBundleGolden_OCI_BakesRepoURL for
the local-output path where OCIParentNamespace is unset, since the current test
only verifies the OCI namespace case. Extend the test or add a companion golden
test that runs Generator.Generate with OCIParentNamespace left empty and asserts
values.yaml preserves repoURL: "" instead of baking a namespace. Use the
existing Generator, Generate, and values.yaml assertions to locate the code.
- Around line 990-1036: The TestBundleGolden_OCI_BakesRepoURL body is using
space indentation instead of Go’s standard tab formatting, which will fail gofmt
and lint checks. Reformat the entire function to match the surrounding style in
argocdhelm_test.go, keeping the existing test logic and symbols like
TestBundleGolden_OCI_BakesRepoURL, Generator, and newRecipeResult unchanged.
In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go`:
- Around line 170-175: The file has mixed spaces and tabs in the
OCIParentNamespace doc block and the Generate body, which will fail gofmt.
Reformat the affected comment and the Generate method in argocdhelm.go to use
standard Go tab indentation consistent with the surrounding declarations,
keeping the same symbols (OCIParentNamespace and Generate) but making the
whitespace gofmt-compliant.
In `@pkg/cli/bundle.go`:
- Around line 216-224: The OCI parent-namespace derivation is duplicated between
parseBundleCmdOptions and printArgoCDHelmOCIInstructions, so extract the shared
“registry + repository minus chart segment” logic into a reusable helper. Prefer
adding a method on oci.Reference such as ParentNamespace() next to ChartName(),
then use that helper from both call sites so the baked repoURL default and the
printed helm hint stay consistent.
🪄 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: d5a835a5-3798-4379-9392-7afda06dd056
📒 Files selected for processing (5)
pkg/bundler/bundler.gopkg/bundler/config/config.gopkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/cli/bundle.go
mohityadav8
force-pushed
the
feat/1342-bake-repourl-oci
branch
from
01969df to
426355a
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
📋 Multi-persona review — bake repoURL default into values.yaml
Method: 4 independent persona reviewers (Correctness · Domain/Architecture · Docs/Contract · CI-DX) → adversarial senior meta-review, each finding re-derived against the resolved code at head cedaaec3. The two blockers were reproduced empirically (go test, gofmt -l).
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
The core mechanic is sound: ParentNamespace() is a faithful, well-tested extraction of the previously-inlined path.Dir logic (all edge cases verified), the two call sites are now provably consistent, the {{ required }} safety-net correctly stays intact for local output, and the new TestBundleGolden_OCI_BakesRepoURL test is good.
But the PR is not mergeable as-is: it fails both mandatory CI gates (make test and make lint), and it ships a user-facing artifact (the generated bundle README) plus reference docs that now assert the opposite of what the code does.
Recommendation: Request changes (2 blockers, both mechanical). Posted as a neutral COMMENT review per author request.
Findings not attached inline (lines outside the diff)
🟠 Major — Generated README + reference docs now claim the opposite of what the code does
pkg/bundler/deployer/argocdhelm/argocdhelm.go:1181-1183 · docs/user/cli-reference.md:1869
The README emitted into every argocd-helm bundle says "The bundle is URL-portable: the publish location is supplied at install time via --set repoURL=..., not baked into the chart bytes." docs/user/cli-reference.md:1869 says "The publish location is not baked into the bundle artifact." After this PR, for OCI-pushed bundles the parent namespace is baked into values.yaml — both statements are now false, and the PR touches no docs (CLAUDE.md requires docs updated in the same PR).
Scope note: the pure "reproducibility invariant" concern is down-weighted — the artifact was already push-target-dependent via bundleChartName = ociRef.ChartName() (#1019), so digests already varied by registry. The concrete defect is the doc/README contradiction. The --set repoURL= override still works, so deploy-time portability is preserved.
Fix: In this PR, correct the generated-README text and docs/user/cli-reference.md (L1869, the step-3 flow ~L1881-1891, the --repo row L1234) plus the echo in docs/user/bundling.md. Reframe as URL-overridable (baked default + --set override) and advertise the new "plain helm install works, no --set repoURL needed" convenience.
🟡 Minor — Generated values.yaml header self-contradicts the baked value
pkg/bundler/deployer/argocdhelm/argocdhelm.go:470-484
The static header const, printed atop every bundle's values.yaml, declares repoURL a "required install-time input" and lists targetRevision as a surfaced key. For an OCI bundle the file now shows a populated repoURL: oci://… right below that "required" wording, and (per Blocker 1) targetRevision no longer appears in the body — the artifact documents contracts that contradict its own contents.
Fix: Branch/soften the header for OCI bundles ("repoURL — pre-filled with the push-target namespace; override with --set repoURL= to deploy from a mirror"), and keep the targetRevision line only if Blocker 1 is fixed by restoring it.
🔵 Nitpick — Dead constant + stale comment for rootValuesTargetRevisionKey
pkg/bundler/deployer/argocdhelm/argocdhelm.go:126-134
If Blocker 1 is fixed by restoring the targetRevision write, this resolves itself. If the drop is kept, rootValuesTargetRevisionKey becomes unreferenced and the comment ("both keys are surfaced … so helm show values documents them") is false — remove the constant and rewrite the comment for repoURL only.
Confirmed non-issues (examined, cleared)
ParentNamespace()correctness — byte-for-byte equivalent to the deleted inline block; verified for single-segment (aicr-bundle→oci://reg), multi-segment, deep-nesting, emptyRepository→"", non-OCI →"".path.Dirnever yields a trailing slash, so the template'strimSuffix "/"/hasPrefix "oci://"branches behave correctly.{{ required }}gate — still fails closed for local output (repoURL: ""→ install errors as before).- CLI plumbing —
ociParentNamespaceset only in the OCI branch; local leaves the zero value.WithTagpreserves registry/repository. - No double-suffix — parent + child templates and
bundleChartNameall use the sameChartName(). verifyBundle— inspects child-Appspec.sources, unaffected by the rootvalues.yamlchange.pkg/bundler/attestationtest failures observed locally (TestNewKeyVerificationIdentity_*) are sigstore TUF network blocks (tuf-repo-cdn.sigstore.dev: Forbidden) — environment, not this PR.
Summary
| Tier | Count | Items |
|---|---|---|
| 🔴 Blocker | 2 | Golden tests fail (dropped targetRevision); gofmt violation |
| 🟠 Major | 1 | Generated README + docs claim "not baked" — now false; docs-in-PR rule |
| 🟡 Minor | 2 | Self-contradicting values.yaml header; redundant --set repoURL hint |
| 🔵 Nitpick | 1 | Dead rootValuesTargetRevisionKey const + stale comment |
The two blockers are one-command fixes (restore one line + gofmt -w); the Major is a same-PR doc sweep. The underlying feature logic is correct.
Reviewed with a multi-persona + adversarial meta-review workflow. Inline comments below anchor the two blockers and the redundant-hint minor to the diff.
| // work without --set flags. For local output, OCIParentNamespace is "" and | ||
| // the {{ required }} safety-net is unchanged. See #1342. | ||
| repoURLDefault := g.OCIParentNamespace | ||
| dynamicOnlyValues[rootValuesRepoURLKey] = repoURLDefault |
There was a problem hiding this comment.
🔴 Blocker — Golden tests fail: the targetRevision default was silently dropped
This block replaced two lines with one — it kept the repoURL write but deleted dynamicOnlyValues[rootValuesTargetRevisionKey] = "". That was the only writer of targetRevision into the root values.yaml, so the key now vanishes from generated output. The golden fixtures testdata/helm_and_manifest_only/values.yaml and testdata/mixed_component/values.yaml still contain the trailing targetRevision: "" line.
Blast radius: Reproduced: go test ./pkg/bundler/deployer/argocdhelm/... → FAIL: TestBundleGolden_HelmAndManifestOnly, FAIL: TestBundleGolden_MixedComponent (byte-diff = the missing targetRevision: "" line). make test / make qualify go red; the PR cannot merge. The PR description's "tests pass" claim does not hold.
Fix: Restore the line rather than regenerate goldens. Dropping targetRevision looks unintentional and regresses the explicit intent of #1020 ("surface both keys so helm show values documents them"). Re-adding dynamicOnlyValues[rootValuesTargetRevisionKey] = "" makes the goldens pass unchanged and keeps the values.yaml header (which still documents targetRevision) consistent. If the drop IS intentional, regenerate goldens with -update AND also fix the stale header (line 470-484) and the now-dead rootValuesTargetRevisionKey constant (line 126-134).
mohityadav8
force-pushed
the
feat/1342-bake-repourl-oci
branch
from
0986d84 to
5b64a39
Compare
|
@njhensley done |
mohityadav8
force-pushed
the
feat/1342-bake-repourl-oci
branch
from
2cc7af8 to
92dd63d
Compare
…ime (argocd-helm deployer) Closes NVIDIA#1342
mohityadav8
force-pushed
the
feat/1342-bake-repourl-oci
branch
from
92dd63d to
4c6f48c
Compare
2 participants
Summary
Bakes the OCI parent namespace as the default
repoURLin the argocd-helm bundle'svalues.yamlat bundle push time.Motivation
When deploying an argocd-helm bundle,
repoURLhad no default — everyhelm installrequired--set repoURL=oci://.... If omitted, child apps deployed with brokenspec.source.repoURLfields.Fixes
Closes #1342
Implementation Notes
ParentNamespace()method onoci.Reference— single source of truth, used by bothparseBundleCmdOptionsandprintArgoCDHelmOCIInstructionsvalues.yamlasrepoURLdefaultrepoURLstays""— existing{{ required }}behaviour unchanged--set repoURL=oci://new-registryTesting
go build ./...✅go test -race ./pkg/bundler/... ./pkg/cli/...✅gofmt -won all changed files ✅