From 1924c07c176564b34702f8c139d04efdf63248b2 Mon Sep 17 00:00:00 2001 From: Yuan Chen Date: Tue, 26 May 2026 12:51:57 -0700 Subject: [PATCH] fix(bundler): tolerate trailing slash on repoURL; quote Chart.yaml version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to PR #1035 (merged) addressing two non-blocking nits from the review that weren't included in the squash: 1. Trailing-slash robustness on path-based child Applications --set repoURL=oci://reg/org/ (an easy copy-paste from a registry UI) used to render as oci://reg/org//aicr-bundle, which Argo CD silently fails to sync against — the double slash hits the OCI resolver as an empty path segment with no operator-facing hint. Pipe .Values.repoURL through trimSuffix "/" before appending /{{ .Chart.Name }} in injectValuesIntoSingleSource so the rendered value is always a clean single-slash artifact reference. Golden fixtures regenerated. 2. Exercise the version branch of writeChartYAML's %q quoting The TestWriteChartYAML_QuotesYAMLReservedScalarsAsName table only covered chart-name reserved scalars. A user-supplied version like "1.0" reparses as the YAML float 1 without the %q wrap on version, so the version-side quoting is load-bearing, not cosmetic. Restructured the table to take an explicit (chartName, chartVersion) pair and added float-looking ("1.0") and numeric-looking ("123") version rows alongside the existing reserved-scalar name rows. Each row now round-trips both fields through yaml.Unmarshal and asserts the exact input string is recovered, so writeChartYAML's version branch is exercised by the same test that documents the rationale. Drive-by: relaxed two pre-existing substring assertions in TestInjectValuesIntoSingleSource that hardcoded `.Values.repoURL }}` to just `.Values.repoURL` so they survive the new trimSuffix pipeline insertion without losing the load-bearing intent (they verify the directive references the caller's value; the exact pipeline shape is documented elsewhere). Related: #1034 (the bug the original #1035 fixed), #1035 (where Mark's review suggested these nits) --- pkg/bundler/deployer/argocdhelm/argocdhelm.go | 2 +- .../deployer/argocdhelm/argocdhelm_test.go | 50 +++++++++++-------- .../templates/nodewright-customizations.yaml | 2 +- .../templates/gpu-operator-post.yaml | 2 +- 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/pkg/bundler/deployer/argocdhelm/argocdhelm.go b/pkg/bundler/deployer/argocdhelm/argocdhelm.go index c442addc7..16b1e40c5 100644 --- a/pkg/bundler/deployer/argocdhelm/argocdhelm.go +++ b/pkg/bundler/deployer/argocdhelm/argocdhelm.go @@ -712,7 +712,7 @@ func injectValuesIntoSingleSource(app map[string]any, overrideKey string) error Kind: yaml.ScalarNode, Tag: yamlStringTag, Style: yaml.SingleQuotedStyle, - Value: `{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}/{{ .Chart.Name }}`, + Value: `{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL | trimSuffix "/" }}/{{ .Chart.Name }}`, } source["targetRevision"] = &yaml.Node{ Kind: yaml.ScalarNode, diff --git a/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go b/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go index 5bdbb16fe..32841f635 100644 --- a/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go +++ b/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go @@ -748,7 +748,7 @@ func TestInjectValuesIntoSingleSource(t *testing.T) { if !strings.Contains(str, `repoURL: '{{ required `) { t.Errorf("source.repoURL should be rewritten to single-quoted Helm directive, got:\n%s", str) } - if !strings.Contains(str, `.Values.repoURL }}`) { + if !strings.Contains(str, `.Values.repoURL`) { t.Errorf("source.repoURL Helm directive should reference .Values.repoURL, got:\n%s", str) } if !strings.Contains(str, `targetRevision: '{{ .Values.targetRevision | default .Chart.Version }}'`) { @@ -1319,8 +1319,13 @@ func TestInjectValuesIntoSingleSource_AppendsChartName(t *testing.T) { } str := string(out) - if !strings.Contains(str, `.Values.repoURL }}/{{ .Chart.Name }}`) { - t.Errorf("path-based child repoURL should append /{{ .Chart.Name }} after .Values.repoURL so the rendered value is the full OCI artifact reference; got:\n%s", str) + // The .Values.repoURL expression may be wrapped in a trimSuffix + // pipeline (defense-in-depth against operator-supplied trailing + // slashes); the load-bearing assertion is that .Chart.Name is + // appended directly so the assembled URL matches the artifact the + // parent App's repoURL/chart:tag triple resolves to. + if !strings.Contains(str, `}}/{{ .Chart.Name }}`) { + t.Errorf("path-based child repoURL should append /{{ .Chart.Name }} after the .Values.repoURL expression so the rendered value is the full OCI artifact reference; got:\n%s", str) } // The error message must direct callers to pass the parent // namespace (the same contract the parent Application uses) so @@ -1340,21 +1345,26 @@ func TestInjectValuesIntoSingleSource_AppendsChartName(t *testing.T) { // is required". func TestWriteChartYAML_QuotesYAMLReservedScalarsAsName(t *testing.T) { tests := []struct { - name string - chartName string - wantUnmarsh string + name string + chartName string + chartVersion string }{ - {"YAML null literal", "null", "null"}, - {"YAML true literal", "true", "true"}, - {"YAML false literal", "false", "false"}, - {"numeric-looking", "123", "123"}, - {"YAML yes literal", "yes", "yes"}, - {"hyphenated normal name", "my-bundle", "my-bundle"}, + {"YAML null literal name", "null", "1.0.0"}, + {"YAML true literal name", "true", "1.0.0"}, + {"YAML false literal name", "false", "1.0.0"}, + {"numeric-looking name", "123", "1.0.0"}, + {"YAML yes literal name", "yes", "1.0.0"}, + // Versions like "1.0" reparse as the YAML float 1 without quoting, + // so the %q wrap on version is load-bearing, not cosmetic. Exercise + // that branch with the same round-trip assertion as for name. + {"float-looking version", "aicr-bundle", "1.0"}, + {"numeric-looking version", "aicr-bundle", "123"}, + {"hyphenated normal pair", "my-bundle", "1.2.3"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { outputDir := t.TempDir() - if _, _, err := writeChartYAML(outputDir, tt.chartName, "1.0.0"); err != nil { + if _, _, err := writeChartYAML(outputDir, tt.chartName, tt.chartVersion); err != nil { t.Fatalf("writeChartYAML: %v", err) } content, err := os.ReadFile(filepath.Join(outputDir, "Chart.yaml")) @@ -1367,16 +1377,16 @@ func TestWriteChartYAML_QuotesYAMLReservedScalarsAsName(t *testing.T) { Version string `yaml:"version"` } if err := yaml.Unmarshal(content, &parsed); err != nil { - t.Fatalf("Chart.yaml does not unmarshal cleanly for name=%q:\n%s\nerror: %v", - tt.chartName, content, err) + t.Fatalf("Chart.yaml does not unmarshal cleanly for name=%q version=%q:\n%s\nerror: %v", + tt.chartName, tt.chartVersion, content, err) } - if parsed.Name != tt.wantUnmarsh { + if parsed.Name != tt.chartName { t.Errorf("Chart.yaml name = %q (after YAML unmarshal), want %q\nraw:\n%s", - parsed.Name, tt.wantUnmarsh, content) + parsed.Name, tt.chartName, content) } - if parsed.Version != "1.0.0" { - t.Errorf("Chart.yaml version = %q, want \"1.0.0\"\nraw:\n%s", - parsed.Version, content) + if parsed.Version != tt.chartVersion { + t.Errorf("Chart.yaml version = %q (after YAML unmarshal), want %q\nraw:\n%s", + parsed.Version, tt.chartVersion, content) } }) } diff --git a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml index abe17b2a1..094502db2 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml @@ -16,7 +16,7 @@ spec: {{- $dynamic := index .Values "nodewrightcustomizations" | default dict -}} {{- toYaml $dynamic | nindent 16 }} path: 002-nodewright-customizations - repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}/{{ .Chart.Name }}' + repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL | trimSuffix "/" }}/{{ .Chart.Name }}' targetRevision: '{{ .Values.targetRevision | default .Chart.Version }}' syncPolicy: automated: diff --git a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml index 3f52f0ce4..037d7da30 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml @@ -16,7 +16,7 @@ spec: {{- $dynamic := index .Values "gpuoperator" | default dict -}} {{- toYaml $dynamic | nindent 16 }} path: 002-gpu-operator-post - repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}/{{ .Chart.Name }}' + repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL | trimSuffix "/" }}/{{ .Chart.Name }}' targetRevision: '{{ .Values.targetRevision | default .Chart.Version }}' syncPolicy: automated: