feat(bundler): URL-portable argocd-helm bundle (#664, #665)#772
Merged
Conversation
Contributor
Contributor
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (2 decrease, 2 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. |
|
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:
📝 WalkthroughWalkthroughThe PR standardizes bundle output to numbered per-component directories (NNN-/), makes Argo CD and argocd-helm generators operate over that layout, and forwards per-component manifests and dynamic values through the bundler via a new ComponentManifests field. The argocd deployer now builds per-folder ApplicationData (BundleDir, IsLocalChart) and emits app-of-apps plus per-component application.yaml files; argocd-helm generates folder-backed Helm templates and a parent Application template for URL-portable bundles. Helm hook annotations are stripped during local Helm rendering. Tests, templates, README, CLI help, and many testdata fixtures were added/updated. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bundler/deployer/argocdhelm/argocdhelm.go (1)
151-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve structured errors from the delegated generator.
argocdGen.Generatecan already return coded*StructuredErrors fromargocd/localformat, but this wraps every failure asErrCodeInternal. That turns invalid-input and timeout paths into generic internal errors for callers. Prefererrors.PropagateOrWrap(genErr, errors.ErrCodeInternal, "failed to generate base Argo CD output")here.Based on learnings
use the idiomatic helper errors.PropagateOrWrap(err, errCode, message)frompkg/errorswhen handling errors in Go code.🤖 Prompt for 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. In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go` around lines 151 - 164, The current error handling after calling argocdGen.Generate wraps all errors as errors.ErrCodeInternal, losing structured error codes; change the return to use errors.PropagateOrWrap(genErr, errors.ErrCodeInternal, "failed to generate base Argo CD output") so existing *StructuredError codes from argocdGen.Generate are preserved or wrapped with ErrCodeInternal when appropriate; locate the call to argocdGen.Generate(ctx, tmpDir) and replace the errors.Wrap(...) usage with errors.PropagateOrWrap(...) while keeping the same message and return semantics.
🤖 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 1145-1146: The flags table for bundle generation is inconsistent
with the text: the prose states that the --repo flag is ignored when --deployer
argocd-helm is used, but the bundle flags table still indicates --repo is used
with argocd-helm; update the bundle flags table entry to match the behavior
described in the paragraph by marking --repo as ignored/no-op for the --deployer
argocd-helm option (or otherwise removing its association), and ensure the
table's description for the --repo and --deployer argocd-helm entries explicitly
note that the publish location is not baked into the bundle and repoURL is
provided at install time via helm install --set repoURL=....
- Line 1113: Add language identifiers to the two fenced code blocks that show
the "bundles/" example so they pass markdownlint rule MD040: update the first
and second code fences (the ones containing the "bundles/" snippet) to start
with ```text instead of ```; keep the content inside unchanged and ensure both
closing fences remain ``` so each fenced block is annotated with the language.
In `@pkg/bundler/deployer/argocd/argocd_test.go`:
- Around line 723-735: The two tests TestGenerate_KustomizeOnly and
TestGenerate_MixedHelmAndKustomize were skipped, removing coverage for the
kustomize/local-chart path; restore them by committing kustomize input fixtures
under testdata/ and corresponding golden output files reflecting the new
localformat NNN-<name>/Chart.yaml + templates/manifest.yaml layout (or update
the test assertions to match the FolderKind/local Helm chart wrapping), remove
the t.Skip calls, and adjust the test expectations in argocd_test.go to assert
the new chart/sources/values.yaml presence and contents so kustomize behavior is
verified again.
In `@pkg/bundler/deployer/argocd/argocd.go`:
- Around line 229-233: The context cancellation inside the for loop (for i, f :=
range folders) currently returns errors.Wrap(errors.ErrCodeInternal, "context
cancelled", ctx.Err()) which masks cancellation as an internal error; change
this to propagate a timeout/cancellation code instead by using the pkg/errors
helper (errors.PropagateOrWrap) or wrapping with errors.ErrCodeTimeout so
callers see a timeout/cancel code (e.g. call errors.PropagateOrWrap(ctx.Err(),
errors.ErrCodeTimeout, "context cancelled") or equivalent) where the current
select case <-ctx.Done() returns.
In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go`:
- Around line 577-625: injectValuesIntoSingleSource currently preserves the
baked-in path (NNN-...) which breaks OCI-published bundles because ArgoCD OCI
Helm chart artifacts must use path: "."; update injectValuesIntoSingleSource
(near variables repoURL, path and the source map) to detect OCI-style repoURLs
(e.g., strings.HasPrefix(repoURL, "oci://")) and, when true, replace
source["path"] with the literal string "." before returning (keep the existing
non-OCI behavior for other repoURL values and retain the current
repoURL/targetRevision template injection).
In `@pkg/bundler/deployer/localformat/hooks_test.go`:
- Around line 23-96: Add a new table-driven test case in the tests slice in
hooks_test.go named like "separator-only documents are ignored" with input set
to a separator-only multi-doc stream (e.g. "---\n# comment\n---\n") and
expectations that no resource content is emitted: set mustNotContain to include
the comment text (e.g. "# comment") and any stray separators (e.g. "---") and
leave mustContain empty (or only assert known absent content), so the test
verifies that parsing/stripping logic in the hook-stripping path does not
produce non-resource documents after re-encoding.
In `@pkg/bundler/deployer/localformat/hooks.go`:
- Around line 55-66: The loop that decodes YAML into a yaml.Node then calls
stripHooksFromDocument and appends to docs can include separator-only or empty
documents which later re-emit useless/invalid content; update the decoding loop
(the decoder.Decode call and subsequent handling) to detect and skip documents
that are empty/contain only separators or have no top-level YAML content before
calling stripHooksFromDocument and before appending to docs (the append(docs,
&doc) path); apply the same empty-document check to the other similar block that
currently appends documents (the second append path referenced in the comment)
so only non-empty resource documents are processed and emitted.
In `@pkg/bundler/deployer/localformat/local_helm.go`:
- Around line 133-137: The current error wrapping in the block calling
stripHelmHooks collapses structured error codes by unconditionally wrapping with
errors.ErrCodeInternal; instead, replace the unconditional errors.Wrap with
errors.PropagateOrWrap so that if stripHelmHooks returns a StructuredError its
existing code is preserved and only unknown errors are wrapped with
errors.ErrCodeInternal—update the code around the call to stripHelmHooks (the
variables stripped, stripErr and the return Folder{} error path) to use
errors.PropagateOrWrap(stripErr, errors.ErrCodeInternal, fmt.Sprintf("strip helm
hooks from manifest %s for %s", p, c.Name)) so existing error codes propagate.
---
Outside diff comments:
In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go`:
- Around line 151-164: The current error handling after calling
argocdGen.Generate wraps all errors as errors.ErrCodeInternal, losing structured
error codes; change the return to use errors.PropagateOrWrap(genErr,
errors.ErrCodeInternal, "failed to generate base Argo CD output") so existing
*StructuredError codes from argocdGen.Generate are preserved or wrapped with
ErrCodeInternal when appropriate; locate the call to argocdGen.Generate(ctx,
tmpDir) and replace the errors.Wrap(...) usage with errors.PropagateOrWrap(...)
while keeping the same message and return semantics.
🪄 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: 806fbb97-f05c-40c3-8f54-fc858c4f58d4
📒 Files selected for processing (41)
docs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/deployer/argocd/argocd.gopkg/bundler/deployer/argocd/argocd_test.gopkg/bundler/deployer/argocd/templates/README.md.tmplpkg/bundler/deployer/argocd/templates/application.yaml.tmplpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/values.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/Chart.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/values.yamlpkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/static/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/static/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yamlpkg/bundler/deployer/localformat/hooks.gopkg/bundler/deployer/localformat/hooks_test.gopkg/bundler/deployer/localformat/local_helm.gopkg/cli/bundle.go
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
9dc4048 to
caa803a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bundler/deployer/argocdhelm/argocdhelm.go (1)
162-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the delegated structured error code here.
argocdGen.Generatecan already return coded invalid-request/timeout errors fromargocdandlocalformat. Wrapping everything asErrCodeInternalhides the real failure mode from callers and CLI exit-code handling.Suggested fix
- if _, genErr := argocdGen.Generate(ctx, tmpDir); genErr != nil { - return nil, errors.Wrap(errors.ErrCodeInternal, "failed to generate base Argo CD output", genErr) - } + if _, genErr := argocdGen.Generate(ctx, tmpDir); genErr != nil { + return nil, errors.PropagateOrWrap(genErr, errors.ErrCodeInternal, "failed to generate base Argo CD output") + }Based on learnings
use the idiomatic helper errors.PropagateOrWrap(err, errCode, message) from pkg/errors.🤖 Prompt for 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. In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go` around lines 162 - 164, The current error wrap hides delegated structured error codes by always using errors.Wrap with errors.ErrCodeInternal; change the return to use the idiomatic helper errors.PropagateOrWrap so delegated codes from argocdGen.Generate are preserved: when argocdGen.Generate(ctx, tmpDir) returns genErr, return errors.PropagateOrWrap(genErr, errors.ErrCodeInternal, "failed to generate base Argo CD output") (keeping the same surrounding logic and return nil first value).
🤖 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 1125-1128: Remove the non-existent upstream.env entry from the
argocd-helm tree example: update the shown tree under the 001-cert-manager/
block so it lists only upstream values files (values.yaml and
cluster-values.yaml) and omits upstream.env, reflecting the actual behavior
where upstream.env is skipped when copying NNN-<name>/ folders into the final
argocd-helm bundle; ensure the example text and any surrounding explanation
mentioning upstream.env are also removed or adjusted.
In `@pkg/bundler/deployer/argocd/templates/README.md.tmpl`:
- Around line 81-84: Update the README template to reflect the actual Argo CD
bundle layout produced by the --deployer argocd path: remove the now-missing
cluster-values.yaml entry and instead document that each local chart directory
under {{ .BundleDir }}/ contains chart metadata (e.g., Chart.yaml or upstream
metadata) and chart contents (templates/ and related files), while keeping the
application.yaml (sync-wave: {{ .SyncWave }}) and values.yaml entries; edit
pkg/bundler/deployer/argocd/templates/README.md.tmpl to mirror the generated
output and mention the per-cluster dynamic values behavior if needed.
In `@pkg/bundler/deployer/argocdhelm/argocdhelm_test.go`:
- Around line 877-934: The test TestHelmTemplate_RendersWithSetRepoURL (and the
similar test around lines 1005-1035) calls helm via
exec.Command(...).CombinedOutput() with no timeout; change these to use a
context with a timeout (e.g., context.WithTimeout) and exec.CommandContext so
the external helm process is bounded, cancel the context on defer, and capture
the CombinedOutput from the command returned by exec.CommandContext; update the
helm invocation sites (the exec.Command("helm","template",...) calls) to use the
ctx-based variant and handle context deadline errors in the test failure
messages.
In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go`:
- Around line 351-359: The required-repoURL hint in parentAppTemplate is
backend-specific (mentions oci://) and should be made backend-agnostic; update
the required string in parentAppTemplate (and any other template constants that
emit the same hint) to remove the OCI-only example and instead show a neutral
placeholder or both formats (e.g., "repoURL is required: pass --set
repoURL=<git-or-oci-url>") so the remediation message applies whether the bundle
is published to Git or an OCI registry.
---
Outside diff comments:
In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go`:
- Around line 162-164: The current error wrap hides delegated structured error
codes by always using errors.Wrap with errors.ErrCodeInternal; change the return
to use the idiomatic helper errors.PropagateOrWrap so delegated codes from
argocdGen.Generate are preserved: when argocdGen.Generate(ctx, tmpDir) returns
genErr, return errors.PropagateOrWrap(genErr, errors.ErrCodeInternal, "failed to
generate base Argo CD output") (keeping the same surrounding logic and return
nil first value).
🪄 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: 507940b7-6e07-45c2-8851-1adc7eb8f1bf
📒 Files selected for processing (42)
docs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/deployer/argocd/argocd.gopkg/bundler/deployer/argocd/argocd_test.gopkg/bundler/deployer/argocd/templates/README.md.tmplpkg/bundler/deployer/argocd/templates/application.yaml.tmplpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/values.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/Chart.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/values.yamlpkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/static/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/static/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yamlpkg/bundler/deployer/localformat/hooks.gopkg/bundler/deployer/localformat/hooks_test.gopkg/bundler/deployer/localformat/local_helm.gopkg/cli/bundle.gotests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
caa803a to
9a0c8ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/user/cli-reference.md (1)
1148-1158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe
argocd-helmtree still shows files that the generated chart no longer emits.This example lists
upstream.envand per-componentcluster-values.yaml, but the new portable chart layout keeps the dynamic surface in the parent chart’s rootvalues.yaml, and the provided testdata here only carries per-component static files. Keeping those entries makes the structure misleading. As per coding guidelines:Ensure accuracy, use neutral tone, clearly structure content, consider audience awareness, and document operational clarity.🤖 Prompt for 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. In `@docs/user/cli-reference.md` around lines 1148 - 1158, The example tree under "argocd-helm" is inaccurate because it lists generated artifacts that the new portable chart layout no longer emits (e.g., upstream.env and per-component cluster-values.yaml); update the example to reflect the new layout by removing entries for upstream.env and per-component cluster-values.yaml and instead show that dynamic configuration lives in the parent chart's root values.yaml (keep only the actual static files present in the provided testdata such as Chart.yaml, templates/, and values.yaml for each component); ensure the tree and any explanatory text reference "parent chart’s root values.yaml" and "static per-component files" to avoid misleading readers.
🤖 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 1249-1269: Remove the misleading cluster-values.yaml entries from
the argocd bundle example: update the tree under the listed bundles
(001-cert-manager, 002-gpu-operator, 003-gpu-operator-post,
004-nodewright-customizations) so none of them show cluster-values.yaml for the
argocd example, and add a brief parenthetical note by the bundle or the
argocd/deployer mention clarifying that --dynamic (cluster-values.yaml) is not
supported with --deployer argocd to keep the example and the earlier statement
consistent.
---
Duplicate comments:
In `@docs/user/cli-reference.md`:
- Around line 1148-1158: The example tree under "argocd-helm" is inaccurate
because it lists generated artifacts that the new portable chart layout no
longer emits (e.g., upstream.env and per-component cluster-values.yaml); update
the example to reflect the new layout by removing entries for upstream.env and
per-component cluster-values.yaml and instead show that dynamic configuration
lives in the parent chart's root values.yaml (keep only the actual static files
present in the provided testdata such as Chart.yaml, templates/, and values.yaml
for each component); ensure the tree and any explanatory text reference "parent
chart’s root values.yaml" and "static per-component files" to avoid misleading
readers.
🪄 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: 24910fcf-a1b6-46e4-9eff-2f904d984905
📒 Files selected for processing (54)
docs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/deployer/argocd/argocd.gopkg/bundler/deployer/argocd/argocd_test.gopkg/bundler/deployer/argocd/templates/README.md.tmplpkg/bundler/deployer/argocd/templates/application.yaml.tmplpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/templates/manifest.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/kustomize_input/cm.yamlpkg/bundler/deployer/argocd/testdata/kustomize_input/kustomization.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/application.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/templates/manifest.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/values.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/values.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/Chart.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/values.yamlpkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/static/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/static/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yamlpkg/bundler/deployer/localformat/hooks.gopkg/bundler/deployer/localformat/hooks_test.gopkg/bundler/deployer/localformat/local_helm.gopkg/cli/bundle.gotests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
9a0c8ff to
7077b6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml (1)
197-215:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
targetRevisionpropagation too.This step passes
--set targetRevision=v1but never verifies thatv1reaches the rendered Argo CD Applications. If.Values.targetRevisionregresses to a default or is dropped entirely, this roundtrip still passes even though the URL-portable install path is broken.Suggested test hardening
echo "$output" | grep -q "oci://example/aicr-bundle" \ || { echo "rendered output should contain user-supplied repoURL" >&2; exit 1; } + echo "$output" | grep -q "targetRevision: v1" \ + || { echo "rendered output should contain user-supplied targetRevision" >&2; exit 1; }🤖 Prompt for 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. In `@tests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml` around lines 197 - 215, Add an assertion that the rendered Argo CD Application contains the propagated targetRevision by checking the captured output variable (output) for the literal "targetRevision: v1" (or the equivalent Application source field showing v1) in the same way repoURL is checked; update the test block around the existing grep for "oci://example/aicr-bundle" to also grep and fail if "targetRevision: v1" is not present so regressions that drop or default .Values.targetRevision are caught.
🤖 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 1136-1160: The example argocd-helm chart tree is missing the
parent Application template; add an entry for templates/aicr-stack.yaml in the
templates/ listing (e.g., place "├── aicr-stack.yaml # Parent
Argo Application" before the other template entries) so the documented layout
matches the flow that references that parent Application (ensure any downstream
text that references templates/aicr-stack.yaml still aligns with the updated
tree).
In `@pkg/bundler/deployer/argocd/argocd.go`:
- Around line 285-289: Replace the current context-cancellation handling in the
select block (the case <-ctx.Done() branch in
pkg/bundler/deployer/argocd/argocd.go) so you propagate the original ctx.Err()
with the cancellation-specific error code instead of ErrCodeInternal; use the
idiomatic helper errors.PropagateOrWrap(ctx.Err(), errors.ErrCodeCanceled,
"context cancelled") (or the project's equivalent cancellation error constant)
and return that error instead of wrapping with ErrCodeInternal.
- Around line 395-396: The IsOCI field is computed using a manual slice check
(len(ref.Source) > 6 && ref.Source[:6] == "oci://") which is inconsistent with
other code; replace that expression with strings.HasPrefix(ref.Source, "oci://")
in the struct initialization for IsOCI (and ensure the strings package is
imported if not already) so the check matches the one used elsewhere (e.g.,
where comp.Source uses strings.HasPrefix).
In `@pkg/bundler/deployer/argocd/templates/README.md.tmpl`:
- Around line 76-104: Update the README template to clarify how child
application.yaml files receive the repository URL: state that if the bundle was
not generated with the --repo option (e.g., aicr bundle --deployer argocd --repo
<URL>), each child {{ .BundleDir }}/application.yaml will contain a placeholder
repo URL and must be updated individually, or instruct users to regenerate with
--repo to populate those URLs; reference the app-of-apps.yaml parent and the
per-component application.yaml files (both for .IsLocalChart and non-local
cases) so users know to check every generated application.yaml under each
NNN-<component>/ folder.
---
Outside diff comments:
In `@tests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml`:
- Around line 197-215: Add an assertion that the rendered Argo CD Application
contains the propagated targetRevision by checking the captured output variable
(output) for the literal "targetRevision: v1" (or the equivalent Application
source field showing v1) in the same way repoURL is checked; update the test
block around the existing grep for "oci://example/aicr-bundle" to also grep and
fail if "targetRevision: v1" is not present so regressions that drop or default
.Values.targetRevision are caught.
🪄 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: b9bfa543-dc29-4412-b5c0-d362381093e6
📒 Files selected for processing (54)
docs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/deployer/argocd/argocd.gopkg/bundler/deployer/argocd/argocd_test.gopkg/bundler/deployer/argocd/templates/README.md.tmplpkg/bundler/deployer/argocd/templates/application.yaml.tmplpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/templates/manifest.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/kustomize_input/cm.yamlpkg/bundler/deployer/argocd/testdata/kustomize_input/kustomization.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/application.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/templates/manifest.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/values.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/values.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/Chart.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/values.yamlpkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/static/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/static/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yamlpkg/bundler/deployer/localformat/hooks.gopkg/bundler/deployer/localformat/hooks_test.gopkg/bundler/deployer/localformat/local_helm.gopkg/cli/bundle.gotests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
7077b6b to
17b60c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bundler/deployer/argocdhelm/argocdhelm.go (1)
151-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the delegated generator's structured error code.
argocdGen.Generate()can already return codedpkg/errorsvalues. Rewrapping every failure here asErrCodeInternalmasks invalid-request and timeout semantics for callers.Based on learnings `use the idiomatic helper errors.PropagateOrWrap(err, errCode, message) from pkg/errors when handling errors in Go code. This helper should be preferred over manual errors.As + conditional return logic`.Suggested fix
if _, genErr := argocdGen.Generate(ctx, tmpDir); genErr != nil { - return nil, errors.Wrap(errors.ErrCodeInternal, "failed to generate base Argo CD output", genErr) + return nil, errors.PropagateOrWrap(genErr, errors.ErrCodeInternal, "failed to generate base Argo CD output") }🤖 Prompt for 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. In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go` around lines 151 - 163, The current error handling around argocdGen.Generate(ctx, tmpDir) wraps any error as ErrCodeInternal and hides the original structured code; change the return to use the package helper errors.PropagateOrWrap(err, errors.ErrCodeInternal, "failed to generate base Argo CD output") so that argocdGen.Generate's original coded error is preserved or wrapped with ErrCodeInternal as appropriate; locate the call to argocdGen.Generate in argocdhelm.go and replace the manual errors.Wrap/rewrap logic with errors.PropagateOrWrap to preserve delegated generator error semantics.
♻️ Duplicate comments (1)
pkg/bundler/deployer/argocd/templates/README.md.tmpl (1)
87-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the extra local-chart files that are still generated.
KindLocalHelmfolders are still emitted withcluster-values.yamlandinstall.shby the shared local-format writer, so omitting them from the tree and calling them “intentionally omitted” no longer matches the bundle contents. That will make generated bundles look wrong to users even when the output is expected.🤖 Prompt for 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. In `@pkg/bundler/deployer/argocd/templates/README.md.tmpl` around lines 87 - 115, The README.md.tmpl currently claims KindLocalHelm folders omit install-time files, but the shared local-format writer still generates cluster-values.yaml and install.sh; update the template (README.md.tmpl) to list these files under the KindLocalHelm branch (e.g., add entries for cluster-values.yaml and install.sh alongside Chart.yaml, templates/, values.yaml, application.yaml) and adjust the explanatory text to state that these install-time artifacts are generated by the local-format writer even though Argo's repo-server does not consume them; reference the KindLocalHelm/ IsLocalChart/ BundleDir/SYNCWAVE symbols shown in the template when making the edit.
🤖 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/deployer/argocd/argocd.go`:
- Around line 250-262: The Argo CD plain generator must reject any dynamic value
usage: before converting components or calling
toLocalformatComponents/localformat.Write (and before forwarding
DynamicPaths/DynamicValues into localformat), check the generator instance's
DynamicValues (or DynamicPaths) flag/property on g (e.g.,
Generator.DynamicValues) and return a clear error if set; alternatively,
explicitly clear/omit DynamicPaths/DynamicValues from the data passed into
toLocalformatComponents/localformat.Write so they are not forwarded. Update the
code around g.toLocalformatComponents, localformat.Write and the
stripUnusedHelmFiles call to perform this validation/clearing and return a
fast-fail error message referencing DynamicValues when detected.
In `@pkg/bundler/deployer/argocdhelm/argocdhelm_test.go`:
- Around line 954-958: The test loop currently breaks on any decoder error (in
the block reading into variable a via dec.Decode(&a)), which hides malformed
YAML; modify the loop to only break when the decoder error is io.EOF and treat
any other error as a test failure (e.g., call t.Fatalf or t.Fatalf-like helper)
so that non-EOF decode errors fail the live-render test explicitly.
In `@pkg/bundler/deployer/localformat/hooks_test.go`:
- Around line 22-146: Add a new table-driven test case in
pkg/bundler/deployer/localformat/hooks_test.go that exercises the error path of
stripHelmHooks by passing a deliberately malformed YAML input (e.g., broken
indentation or truncated mapping) and asserting that stripHelmHooks returns a
non-nil error; implement it as a separate test function (e.g.,
TestStripHelmHooks_MalformedYAML) or add to the existing tests loop, referencing
stripHelmHooks so the test fails if the function silently returns pass-through
output instead of an error, and ensure the test checks error != nil rather than
inspecting output content.
In `@pkg/bundler/deployer/localformat/hooks.go`:
- Around line 116-124: The current loop in hooks.go that builds filtered (from
annotations.Content) removes any annotation whose key.Value has the prefix
"helm.sh/hook", which strips delete-phase hooks too; change the condition so you
only skip (i.e., remove) annotations when key.Value is "helm.sh/hook" AND the
annotation value lists one of the sync-phase hooks you want to remove (e.g.,
"post-install", "pre-upgrade", "post-upgrade", "pre-install", "pre-rollback",
etc.), while preserving annotations that include "pre-delete" or "post-delete";
implement this by parsing key.Value (or the paired val.Value) and checking
membership in an explicit allowlist of hook types before calling continue,
leaving all other helm.sh/hook annotations appended to filtered.
---
Outside diff comments:
In `@pkg/bundler/deployer/argocdhelm/argocdhelm.go`:
- Around line 151-163: The current error handling around argocdGen.Generate(ctx,
tmpDir) wraps any error as ErrCodeInternal and hides the original structured
code; change the return to use the package helper errors.PropagateOrWrap(err,
errors.ErrCodeInternal, "failed to generate base Argo CD output") so that
argocdGen.Generate's original coded error is preserved or wrapped with
ErrCodeInternal as appropriate; locate the call to argocdGen.Generate in
argocdhelm.go and replace the manual errors.Wrap/rewrap logic with
errors.PropagateOrWrap to preserve delegated generator error semantics.
---
Duplicate comments:
In `@pkg/bundler/deployer/argocd/templates/README.md.tmpl`:
- Around line 87-115: The README.md.tmpl currently claims KindLocalHelm folders
omit install-time files, but the shared local-format writer still generates
cluster-values.yaml and install.sh; update the template (README.md.tmpl) to list
these files under the KindLocalHelm branch (e.g., add entries for
cluster-values.yaml and install.sh alongside Chart.yaml, templates/,
values.yaml, application.yaml) and adjust the explanatory text to state that
these install-time artifacts are generated by the local-format writer even
though Argo's repo-server does not consume them; reference the KindLocalHelm/
IsLocalChart/ BundleDir/SYNCWAVE symbols shown in the template when making the
edit.
🪄 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: 68632e40-570f-40d2-a39f-4aff63eae3af
📒 Files selected for processing (54)
docs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/deployer/argocd/argocd.gopkg/bundler/deployer/argocd/argocd_test.gopkg/bundler/deployer/argocd/templates/README.md.tmplpkg/bundler/deployer/argocd/templates/application.yaml.tmplpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/templates/manifest.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/002-my-kustomize-app/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_kustomize/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/application.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/002-nodewright-customizations/values.yamlpkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/kustomize_input/cm.yamlpkg/bundler/deployer/argocd/testdata/kustomize_input/kustomization.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/application.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/templates/manifest.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/001-my-kustomize-app/values.yamlpkg/bundler/deployer/argocd/testdata/kustomize_only/app-of-apps.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/001-gpu-operator/values.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/Chart.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/application.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocd/testdata/mixed_component/002-gpu-operator-post/values.yamlpkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/001-cert-manager/values.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/002-nodewright-customizations/templates/tuning.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/static/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/cert-manager.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yamlpkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/002-gpu-operator-post/templates/dcgm-exporter.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/static/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/aicr-stack.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator.yamlpkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yamlpkg/bundler/deployer/localformat/hooks.gopkg/bundler/deployer/localformat/hooks_test.gopkg/bundler/deployer/localformat/local_helm.gopkg/cli/bundle.gotests/chainsaw/cli/bundle-dynamic/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
17b60c0 to
96231df
Compare
mchmarny
reviewed
May 6, 2026
mchmarny
left a comment
mchmarny
left a comment
Member
There was a problem hiding this comment.
Solid rewrite — fixes three real bugs (literal-block render, silent-drop of manifest-only/mixed components, baked-in URLs) with strong test coverage including live helm template round-trips and a fail-without-repoURL negative test. Architecture of delegating to the argocd.Generator and wrapping is clean; AllowDynamicValueSplit is the right opt-in boundary; the stripHelmHooks rationale doc-comment is exemplary.
Three findings worth addressing — none block merge:
- New chainsaw helm-template steps assume
helmis on PATH. --reposilently disappears for--deployer argocd-helmat runtime; help text mentions it but no log line.cli-reference.mdflag table contradicts the new prose on--repoportability.
CI is green.
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
96231df to
574d3cd
Compare
mchmarny
previously approved these changes
May 7, 2026
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
4b5b393 to
cfeab05
Compare
mchmarny
approved these changes
May 7, 2026
lockwobr
force-pushed
the
feat/argocd-helm-url-portable
branch
from
cfeab05 to
0f108c7
Compare
25 tasks
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
Make the argocd-helm bundle a build-once, URL-portable artifact. Closes #664 and #665, plus fixes the original
values: |-literal-block render bug that was blocking every install.Motivation / Context
Three intertwined goals:
nindent 8for the merged values block scalar. Withvalues: |-at column 12, that placed content outside the literal block, so kubectl rejected every Application with "field not declared in schema". The bundle simply did not install.--deployer argocdand--deployer argocd-helm. Recipes that includednodewright-customizationsor any mixed component (Helm chart + raw manifests likegpu-operator'sdcgm-exporter) lost those resources entirely.application.yamlreferencedoci://<repo>at bundle time, so re-publishing to a different registry required regenerating the bundle.Fixes: #664, #665
Related: #516 (umbrella), #515 (dynamic install-time values)
Type of Change
Component(s) Affected
pkg/bundler,pkg/component/*)cmd/aicr,pkg/cli) —--repodeprecation note for--deployer argocd-helmdocs/,examples/)pkg/recipe) — no logic changes; consumed vialocalformatImplementation Notes
#664— argocd deployer adopts NNN-folder layout vialocalformat.WriteThe argocd deployer no longer hand-rolls per-component directories. It calls the same
localformat.Writethe helm deployer uses, gettingNNN-<name>/Chart.yaml + templates/(orupstream.env) for free. argocd then writes a singleapplication.yamlinside each folder. The Application shape branches onlocalformat.FolderKind:KindUpstreamHelm(noChart.yaml) → today's multi-source upstream-helm shape, unchanged.KindLocalHelm(Chart.yaml+templates/present) → new path-based single-source shape (source.path: NNN-<name>).This is what makes manifest-only and mixed-component raw manifests deploy: localformat wraps them as local Helm charts, argocd points an Application at them via
path:.#665— argocd-helm transforms both shapestransformApplicationdetectsspec.sources(multi) vsspec.source(single) and routes to the right transformation:helm.valuesmerging static (.Files.Get) and dynamic (.Values.<key>) — with thenindent 16fix for the original render bug.injectValuesIntoSingleSourceadds ahelm.valuesblock exposing dynamic.Values.<key>overrides (the wrapped chart's ownvalues.yamlis the static layer).URL-portable bundle
Parent Application moved from a hard-coded
application.yamlat bundle root intotemplates/aicr-stack.yaml, a Helm template using{{ .Values.repoURL }}/{{ .Values.targetRevision }}. Path-based child Applications also use.Values.repoURL/.Values.targetRevision(replaced via single-quoted yaml.Node directives so embedded"..."inrequiredsurvives marshaling). Same artifact bytes work for any registry — URL supplied once at install time:Helm hooks stripped from wrapped manifests
localformat.stripHelmHookswalks rendered manifest YAML and removeshelm.sh/hook*annotations before they reachtemplates/. Reason: Argo's Helm processor mapshelm.sh/hook: post-install→argocd.argoproj.io/hook: PostSync, treating the resource as a hook. WithsyncPolicy.automated, Argo only runs sync operations when something is out-of-sync; a chart whose only resources are hook-annotated reports trivially "Synced" and the hook never fires. Skyhook CRs and similar would silently never apply. The NNN-prefix folder ordering at the bundle layer subsumes the role helm hooks were playing for these wrapped charts.OCI URL/version handling
argocd.buildApplicationDatanow appends the chart-name segment to OCIrepoURL(mirroringlocalformat.writeUpstreamHelmFolder's convention) and preserves thevprefix on OCI tags —v1.3.0is a literal tag inghcr.io, not equivalent to1.3.0. Without this,nvsentinelandkai-schedulerfailed with "manifests/: not found" against the registry. Recipe data unchanged.ServerSideApply=trueon Application syncOptionsSeveral upstream charts ship CRDs that exceed kubectl's 262144-byte client-side annotation cap.
kai-scheduler'sconfigs.kai.schedulerCRD is 721,579 bytes — 2.75× over. Argo's default client-side apply writes the full resource JSON intokubectl.kubernetes.io/last-applied-configuration, hitting the apiserver's annotation field limit. WithoutServerSideApply=true, sync retries forever withmetadata.annotations: Too long: may not be more than 262144 bytes.Testing
make qualify go test -race ./pkg/bundler/... golangci-lint run -c .golangci.yaml ./pkg/bundler/...Test surface added in this PR:
testdata/helm_and_manifest_only/,testdata/mixed_component/) freezing the bundle layout. Regenerate withgo test ... -run TestBundleGolden -args -update.helm templateintegration tests —TestHelmTemplate_RendersWithSetRepoURLruns the chart through Helm with--set repoURL=...and asserts the rendered parent App + path-based child both pick up the user-supplied URL.TestHelmTemplate_FailsWithoutRepoURLverifies therequireddirective fires whenrepoURLis omitted. Botht.Skipifhelmnot onPATH.TestBuildApplicationData_OCIHandling— table-driven (4 cases): nvsentinel pattern (chart != org), kai-scheduler pattern (chart == org), trailing-slash edge case, HTTPS pass-through.TestStripHelmHooks— table-driven (4 cases) plus an empty-input pass-through test.TestInjectValuesIntoSingleSourceextended to assert URL-portability invariants (templatedrepoURL/targetRevision, single-quoted-style preserved, baked-in input URL absent from output).t.Skip-ed — they need testdata fixtures and assertion rewrites for the wrapped-chart layout. Tracked under [Feature]: --deployer argocd adopts uniform local-chart layout #664 follow-up.End-to-end verified against a fresh KWOK cluster (
make kwok-cluster && make kwok-nodes RECIPE=h100-eks-ubuntu-training):aicr-stackdeploy viahelm install ... --set repoURL=....tuning) auto-applied without manual sync — helm-hook strip working.nvsentinelandkai-scheduler(previouslyUnknowndue to OCI handling) now resolve correctly.kai-schedulerandkube-prometheus-stackinstall cleanly viaServerSideApply=true(previously stuck in retry loops).Risk Assessment
Rollout notes:
Three breaking changes; documented in the user-facing CLI reference and the bundle's generated README:
--deployer argocdbundle layout — flat<name>/application.yaml + values.yaml→NNN-<name>/{Chart.yaml or upstream.env, application.yaml, values.yaml, cluster-values.yaml, ...}. Pure-Helm Application shape unchanged; manifest-only and mixed components newly deploy (previously silently broken). External tools that parsed bundle paths by bare component name need to account for theNNN-prefix.--deployer argocd-helmbundle root no longer emitsapplication.yaml— the parent Application istemplates/aicr-stack.yamland is rendered by Helm at install time using.Values.repoURL/.Values.targetRevision. Users who previously didkubectl apply -f bundle/application.yamlwill insteadhelm install aicr-bundle <chart-source> --set repoURL=... --set targetRevision=....--repoflag is now ignored by--deployer argocd-helm(the URL is supplied at install time, not at bundle time). Help text updated; flag still functional for--deployer argocd.No migration script needed — bundles auto-regenerate on every
aicr bundle. Existing in-cluster Applications keep working until a re-bundle/redeploy.Checklist
make testwith-race)make lint) — Go lint clean; yamllint hits a local-worktree-only false positive(
.claude/worktrees/), CI unaffectedgit commit -S) — you'll handle on commit