Skip to content

fix(bundler): quote argocd app-of-apps metadata.name (#1011)#1040

Merged
mchmarny merged 3 commits into
NVIDIA:mainfrom
yuanchen8911:fix/argocd-app-name-yaml-quoting-1036
May 27, 2026
Merged

fix(bundler): quote argocd app-of-apps metadata.name (#1011)#1040
mchmarny merged 3 commits into
NVIDIA:mainfrom
yuanchen8911:fix/argocd-app-name-yaml-quoting-1036

Conversation

@yuanchen8911
Copy link
Copy Markdown
Contributor

Summary

Quote metadata.name in the static argocd app-of-apps.yaml template so a --app-name value that is a DNS-1123-valid YAML-reserved scalar (123, true, false, null, 1e3, …) renders as a string instead of a number/bool/null. Brings the static argocd template in line with the argocd-helm parent template (which already uses Sprig | quote).

Motivation / Context

Follow-up to #1036, which introduced --app-name. The argocd deployer emitted metadata.name: {{ .AppName }} without quoting. ValidateAppName delegates to k8s.io/apimachinery's IsDNS1123Subdomain, which legitimately accepts digit-only labels and bare YAML keywords (123, true, null, 1e3). With the unquoted template, aicr bundle --deployer argocd --app-name 123 ... rendered:

metadata:
  name: 123

sigs.k8s.io/yaml.YAMLToJSON (used by kubectl) routes through JSON and emits "name":123 (number, not string). unstructured.GetName() then returns "" because getNestedString does a val.(string) type assertion on the int64, so kubectl apply is rejected with a cryptic metadata-name validation error far from the original aicr bundle call site. The sibling argocd-helm parent template at pkg/bundler/deployer/argocdhelm/argocdhelm.go:446 already applies | quote.

Fixes: N/A (cross-review follow-up to merged #1036)
Related: #1011, #1036

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Component(s) Affected

  • Bundlers (pkg/bundler, pkg/component/*)

Implementation Notes

  • Template change: name: {{ .AppName }}name: {{ printf "%q" .AppName }}. Go text/template has no quote pipe; printf "%q" produces a Go-style double-quoted string, which is a valid YAML string scalar for the DNS-1123 character set enforced by ValidateAppName.
  • The static argocd deployer materializes a manifest at bundle time, so this fix has no install-time analogue — the value has to be safe at render time.
  • The three regenerated goldens reflect the same name: "nvidia-stack" (quoted) form. The existing TestGenerate_AppName assertion is tightened from name: <value> to name: "<value>" so a future regression cannot silently revert.
  • Defense-in-depth ValidateAppName is unchanged. Tightening the validator to reject digit-only / YAML-keyword names would break operators who legitimately want them; the template fix is the narrower, less surprising option.

Testing

# Scoped — full make qualify currently hits unrelated macOS sandbox failures (mktemp blocked)
GOFLAGS="-mod=vendor" go test -race -count=1 ./pkg/bundler/deployer/argocd/...   # PASS
golangci-lint run -c .golangci.yaml ./pkg/bundler/deployer/argocd/...            # 0 issues

New test TestGenerate_AppName_YAMLReservedScalars rebuilds the kubectl decode path (sigs.k8s.io/yaml.YAMLToJSONunstructured.UnmarshalJSONGetName()) and asserts the rendered metadata.name round-trips as a string for 123, true, false, null, 1e3. Without the template fix, every subtest fails because GetName() returns "".

Coverage delta (pkg/bundler/deployer/argocd): expected to increase slightly with the new subtests; full make qualify will run in CI.

Risk Assessment

  • Low — Single-line template change behind defense-in-depth validation, three golden updates, and one new regression test. Default nvidia-stack rendering is unchanged behaviorally (name: nvidia-stack and name: "nvidia-stack" are equivalent YAML strings).

Rollout notes: None. The rendered manifest is character-different (name: "nvidia-stack" vs name: nvidia-stack) but semantically identical for any YAML/JSON parser. Argo CD's argocd app get/sync examples in the README are unaffected (already plain text).

Checklist

  • Tests pass locally (make test with -race) — package-scoped; see Testing
  • Linter passes (make lint) — package-scoped; see Testing
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality
  • I updated docs if user-facing behavior changed — N/A (no user-facing behavior change)
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S)

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 1482d745-9dfd-4121-9d24-f73a83320c1b

📥 Commits

Reviewing files that changed from the base of the PR and between a4b152f and db68b4b.

📒 Files selected for processing (5)
  • pkg/bundler/deployer/argocd/argocd_test.go
  • pkg/bundler/deployer/argocd/templates/app-of-apps.yaml.tmpl
  • pkg/bundler/deployer/argocd/testdata/helm_and_kustomize/app-of-apps.yaml
  • pkg/bundler/deployer/argocd/testdata/helm_and_manifest_only/app-of-apps.yaml
  • pkg/bundler/deployer/argocd/testdata/kustomize_only/app-of-apps.yaml

📝 Walkthrough

Walkthrough

This PR quotes Argo CD Application metadata.name in the app-of-apps template using Go's printf "%q". Tests were updated: an existing test now expects a quoted name and a new test renders several YAML-reserved-scalar AppName values, converts the generated YAML to JSON, unmarshals into an unstructured.Unstructured, and asserts GetName() equals the original string. Three testdata fixtures were adjusted to use quoted metadata.name values.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

size/M

Suggested reviewers

  • lockwobr
  • mchmarny
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: quoting metadata.name in Argo CD's app-of-apps template to handle YAML-reserved scalars correctly.
Description check ✅ Passed The description provides clear context on the bug, motivation, implementation details, testing, and risk assessment—fully related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

The argocd deployer's app-of-apps.yaml.tmpl emitted `name: {{ .AppName }}`
unquoted. `ValidateAppName` delegates to k8s.io/apimachinery's DNS-1123
subdomain check, which legitimately accepts digit-only labels (`123`) as
well as YAML-reserved scalars like `true`, `false`, `null`, and `1e3`.

With the unquoted template, `aicr bundle --deployer argocd --app-name 123`
rendered:

  metadata:
    name: 123

`sigs.k8s.io/yaml.YAMLToJSON` (used by kubectl) routes through JSON and
emits `"name":123` (number). `unstructured.GetName()` then returns ""
because of the val.(string) type assertion inside getNestedString, so
apply is rejected with a metadata.name validation error far from the
original `aicr bundle` call site.

The sibling argocd-helm parent template at argocdhelm.go:446 already
applies Sprig `| quote`. This change brings the static argocd template
into line by using `printf "%q"` (text/template has no `quote` pipe).

Regression test `TestGenerate_AppName_YAMLReservedScalars` rebuilds the
kubectl decode path (sigs.k8s.io/yaml → unstructured.UnmarshalJSON →
GetName) and asserts the rendered metadata.name round-trips as a string
for `123`, `true`, `false`, `null`, and `1e3`. The existing
TestGenerate_AppName assertion is tightened to require quoted form.
Three goldens regenerated.
@yuanchen8911 yuanchen8911 force-pushed the fix/argocd-app-name-yaml-quoting-1036 branch from a4b152f to db68b4b Compare May 27, 2026 00:17
@mchmarny mchmarny assigned mchmarny and unassigned yuanchen8911 May 27, 2026
@mchmarny mchmarny self-requested a review May 27, 2026 11:18
mchmarny added a commit that referenced this pull request May 27, 2026
PR #1027 bumped testing_tools.helmfile v1.5.1 -> v1.5.2 but the
Renovate postUpgradeTask that refreshes helmfile_checksums did not
run, leaving the four per-arch SHA256 entries pinned at v1.5.1
values. Both .github/actions/install-e2e-tools (via tools/setup-tools)
and .github/actions/setup-build-tools verify the downloaded helmfile
tarball against these pinned checksums, so E2E and CLI E2E started
failing on every PR rebased onto main after #1027 merged (PR #1040
and others).

Refresh the four checksums by running
tools/update-helmfile-checksums v1.5.2, which is the same script the
Renovate postUpgradeTask invokes. No other code or doc references
the stale digests.
@mchmarny mchmarny merged commit ceec36e into NVIDIA:main May 27, 2026
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants