From 4bdb6f097c2937d3214efef3c287c9ed58fe7183 Mon Sep 17 00:00:00 2001 From: Etienne Champetier Date: Thu, 26 Mar 2026 01:16:54 -0400 Subject: [PATCH] fix: keep all chart dependencies key / values (#2501) * feat: Refactor TestRewriteChartDependencies Signed-off-by: Etienne Champetier * fix: keep all chart dependencies key / values In rewriteChartDependencies we were only parsing name / repository / version, thus dropping keys like condition / import-values. This at least fixes the use of condition. Signed-off-by: Etienne Champetier --------- Signed-off-by: Etienne Champetier --- pkg/state/chart_dependencies_rewrite_test.go | 128 ++++++++++-------- pkg/state/state.go | 6 +- test/integration/run.sh | 1 + .../test-cases/chart-deps-condition.sh | 13 ++ .../input/chart/Chart.yaml | 14 ++ .../input/chart/charts/subchart1/Chart.yaml | 6 + .../chart/charts/subchart1/templates/cm.yaml | 6 + .../input/chart/charts/subchart2/Chart.yaml | 6 + .../chart/charts/subchart2/templates/cm.yaml | 6 + .../input/chart/values.yaml | 2 + .../chart-deps-condition/input/helmfile.yaml | 57 ++++++++ .../chart-deps-condition/output/template | 86 ++++++++++++ 12 files changed, 274 insertions(+), 57 deletions(-) create mode 100644 test/integration/test-cases/chart-deps-condition.sh create mode 100644 test/integration/test-cases/chart-deps-condition/input/chart/Chart.yaml create mode 100644 test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/Chart.yaml create mode 100644 test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/templates/cm.yaml create mode 100644 test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/Chart.yaml create mode 100644 test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/templates/cm.yaml create mode 100644 test/integration/test-cases/chart-deps-condition/input/chart/values.yaml create mode 100644 test/integration/test-cases/chart-deps-condition/input/helmfile.yaml create mode 100644 test/integration/test-cases/chart-deps-condition/output/template diff --git a/pkg/state/chart_dependencies_rewrite_test.go b/pkg/state/chart_dependencies_rewrite_test.go index 09ebc893..a51ae31a 100644 --- a/pkg/state/chart_dependencies_rewrite_test.go +++ b/pkg/state/chart_dependencies_rewrite_test.go @@ -17,7 +17,7 @@ func TestRewriteChartDependencies(t *testing.T) { chartYaml string expectModified bool expectError bool - validate func(t *testing.T, chartPath string) + validate func(t *testing.T, modifiedChartYaml string) }{ { name: "no Chart.yaml exists", @@ -46,13 +46,8 @@ dependencies: `, expectModified: false, expectError: false, - validate: func(t *testing.T, chartPath string) { - data, err := os.ReadFile(filepath.Join(chartPath, "Chart.yaml")) - if err != nil { - t.Fatalf("failed to read Chart.yaml: %v", err) - } - content := string(data) - if !strings.Contains(content, "file:///absolute/path/to/chart") { + validate: func(t *testing.T, modifiedChartYaml string) { + if !strings.Contains(modifiedChartYaml, "file:///absolute/path/to/chart") { t.Errorf("absolute path should not be modified") } }, @@ -69,20 +64,14 @@ dependencies: `, expectModified: true, expectError: false, - validate: func(t *testing.T, chartPath string) { - data, err := os.ReadFile(filepath.Join(chartPath, "Chart.yaml")) - if err != nil { - t.Fatalf("failed to read Chart.yaml: %v", err) - } - content := string(data) - + validate: func(t *testing.T, modifiedChartYaml string) { // Should have been converted to absolute path - if strings.Contains(content, "file://../relative-chart") { + if strings.Contains(modifiedChartYaml, "file://../relative-chart") { t.Errorf("relative path should have been converted to absolute") } // Should now have an absolute path - if !strings.Contains(content, "file://") { + if !strings.Contains(modifiedChartYaml, "file://") { t.Errorf("should still have file:// prefix") } }, @@ -108,30 +97,24 @@ dependencies: `, expectModified: true, expectError: false, - validate: func(t *testing.T, chartPath string) { - data, err := os.ReadFile(filepath.Join(chartPath, "Chart.yaml")) - if err != nil { - t.Fatalf("failed to read Chart.yaml: %v", err) - } - content := string(data) - + validate: func(t *testing.T, modifiedChartYaml string) { // HTTPS repo should remain unchanged - if !strings.Contains(content, "https://charts.example.com") { + if !strings.Contains(modifiedChartYaml, "https://charts.example.com") { t.Errorf("https repository should not be modified") } // Relative path should be converted - if strings.Contains(content, "file://../relative-chart") { + if strings.Contains(modifiedChartYaml, "file://../relative-chart") { t.Errorf("relative file:// path should have been converted") } // Absolute path should remain unchanged - if !strings.Contains(content, "file:///absolute/chart") { + if !strings.Contains(modifiedChartYaml, "file:///absolute/chart") { t.Errorf("absolute file:// path should not be modified") } // OCI repo should remain unchanged - if !strings.Contains(content, "oci://registry.example.com") { + if !strings.Contains(modifiedChartYaml, "oci://registry.example.com") { t.Errorf("oci repository should not be modified") } }, @@ -154,21 +137,50 @@ dependencies: `, expectModified: true, expectError: false, - validate: func(t *testing.T, chartPath string) { - data, err := os.ReadFile(filepath.Join(chartPath, "Chart.yaml")) - if err != nil { - t.Fatalf("failed to read Chart.yaml: %v", err) - } - content := string(data) - + validate: func(t *testing.T, modifiedChartYaml string) { // All relative paths should be converted - if strings.Contains(content, "file://../chart1") || - strings.Contains(content, "file://./chart2") || - strings.Contains(content, "file://../../../chart3") { + if strings.Contains(modifiedChartYaml, "file://../chart1") || + strings.Contains(modifiedChartYaml, "file://./chart2") || + strings.Contains(modifiedChartYaml, "file://../../../chart3") { t.Errorf("all relative paths should have been converted") } }, }, + { + name: "extra fields", + chartYaml: `apiVersion: v2 +name: test-chart +version: 1.0.0 +dependencies: + - name: dep1 + repository: https://charts.example.com + version: 1.0.0 + condition: dep.install + import-values: + - child: persistence + parent: global.persistence + extra-field2: bbb +extra-field: aaa +`, + expectModified: false, + expectError: false, + validate: func(t *testing.T, modifiedChartYaml string) { + requiredFields := []string{ + "condition: dep.install", + "import-values:", + "child: persistence", + "parent: global.persistence", + "extra-field2: bbb", + "extra-field: aaa", + } + + for _, field := range requiredFields { + if !strings.Contains(modifiedChartYaml, field) { + t.Errorf("field %q should be preserved", field) + } + } + }, + }, } for _, tt := range tests { @@ -216,26 +228,34 @@ dependencies: t.Fatalf("unexpected error: %v", err) } + if tt.chartYaml == "" { + return + } + + modifiedChartBytes, err := os.ReadFile(filepath.Join(tempDir, "Chart.yaml")) + if err != nil { + t.Fatalf("failed to read Chart.yaml: %v", err) + } + modifiedChartYaml := string(modifiedChartBytes) + // Validate the modified Chart.yaml if tt.validate != nil { - tt.validate(t, tempDir) + tt.validate(t, modifiedChartYaml) } // Call cleanup and verify restoration - if tt.chartYaml != "" { - cleanup() + cleanup() - // Read restored content - restoredContent, err := os.ReadFile(chartYamlPath) - if err != nil { - t.Fatalf("failed to read restored Chart.yaml: %v", err) - } + // Read restored content + restoredContent, err := os.ReadFile(chartYamlPath) + if err != nil { + t.Fatalf("failed to read restored Chart.yaml: %v", err) + } - // Verify content was restored - if string(restoredContent) != string(originalContent) { - t.Errorf("cleanup did not restore original content\noriginal:\n%s\nrestored:\n%s", - string(originalContent), string(restoredContent)) - } + // Verify content was restored + if string(restoredContent) != string(originalContent) { + t.Errorf("cleanup did not restore original content\noriginal:\n%s\nrestored:\n%s", + string(originalContent), string(restoredContent)) } }) } @@ -319,6 +339,7 @@ dependencies: - name: dep1 repository: file://../relative-chart version: 1.0.0 + condition: test ` chartYamlPath := filepath.Join(tempDir, "Chart.yaml") @@ -346,9 +367,7 @@ dependencies: content := string(modifiedContent) - // Verify top-level fields are preserved - // Note: The implementation preserves top-level chart metadata but may not preserve - // extra dependency-level fields (like condition, tags) that are not in the ChartDependency struct + // Verify fields are preserved requiredFields := []string{ "apiVersion: v2", "name: test-chart", @@ -356,6 +375,7 @@ dependencies: "description: A test chart", "keywords:", "maintainers:", + "condition:", } for _, field := range requiredFields { diff --git a/pkg/state/state.go b/pkg/state/state.go index 27b48dd0..9fe36558 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1444,9 +1444,9 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error) // Parse Chart.yaml type ChartDependency struct { - Name string `yaml:"name"` - Repository string `yaml:"repository"` - Version string `yaml:"version"` + Name string `yaml:"name"` + Repository string `yaml:"repository"` + Data map[string]interface{} `yaml:",inline"` } type ChartMeta struct { Dependencies []ChartDependency `yaml:"dependencies,omitempty"` diff --git a/test/integration/run.sh b/test/integration/run.sh index 6826406c..a7116e8f 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -96,6 +96,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes # TEST CASES---------------------------------------------------------------------------------------------------------- +. ${dir}/test-cases/chart-deps-condition.sh . ${dir}/test-cases/fetch-forl-local-chart.sh . ${dir}/test-cases/suppress-output-line-regex.sh . ${dir}/test-cases/chartify-jsonPatches-and-strategicMergePatches.sh diff --git a/test/integration/test-cases/chart-deps-condition.sh b/test/integration/test-cases/chart-deps-condition.sh new file mode 100644 index 00000000..83b0efde --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition.sh @@ -0,0 +1,13 @@ +chart_deps_condition_input_dir="${cases_dir}/chart-deps-condition/input" + +chart_deps_condition_tmp=$(mktemp -d) +actual=${chart_deps_condition_tmp}/actual.yaml +expected="${cases_dir}/chart-deps-condition/output/template" + +test_start "chart with dependencies and condition" + +# --concurrency 1 is a workaround for https://github.com/helmfile/helmfile/issues/2502 +${helmfile} -f ${chart_deps_condition_input_dir}/helmfile.yaml template --skip-deps --concurrency 1 > ${actual} || fail "\"helmfile template\" shouldn't fail" +./dyff between -bs ${expected} ${actual} || fail "\"helmfile template\" should be consistent" + +test_pass "chart with dependencies and condition" diff --git a/test/integration/test-cases/chart-deps-condition/input/chart/Chart.yaml b/test/integration/test-cases/chart-deps-condition/input/chart/Chart.yaml new file mode 100644 index 00000000..4da6b8c5 --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/input/chart/Chart.yaml @@ -0,0 +1,14 @@ +dependencies: + - name: subchart1 + repository: file://charts/subchart1 + condition: subchart1.install + version: 1.0.0 + - name: subchart2 + repository: file://charts/subchart2 + version: 2.0.0 +apiVersion: v2 +appVersion: 3.0.0 +description: Test helm charts dependencies +name: testchartsdeps +type: application +version: 3.0.0 diff --git a/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/Chart.yaml b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/Chart.yaml new file mode 100644 index 00000000..9d82a5c9 --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/Chart.yaml @@ -0,0 +1,6 @@ +apiVersion: v2 +name: subchart1 +description: Test sub helm chart 1 +type: application +version: 1.0.0 +appVersion: "1.0.0" diff --git a/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/templates/cm.yaml b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/templates/cm.yaml new file mode 100644 index 00000000..dd3d1095 --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart1/templates/cm.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: testsub1 +data: + sub: 1 diff --git a/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/Chart.yaml b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/Chart.yaml new file mode 100644 index 00000000..53c41e13 --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/Chart.yaml @@ -0,0 +1,6 @@ +apiVersion: v2 +name: subchart2 +description: Test sub helm chart 2 +type: application +version: 2.0.0 +appVersion: "2.0.0" diff --git a/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/templates/cm.yaml b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/templates/cm.yaml new file mode 100644 index 00000000..01084d75 --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/input/chart/charts/subchart2/templates/cm.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: testsub2 +data: + sub: 2 diff --git a/test/integration/test-cases/chart-deps-condition/input/chart/values.yaml b/test/integration/test-cases/chart-deps-condition/input/chart/values.yaml new file mode 100644 index 00000000..687024b1 --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/input/chart/values.yaml @@ -0,0 +1,2 @@ +subchart1: + install: true diff --git a/test/integration/test-cases/chart-deps-condition/input/helmfile.yaml b/test/integration/test-cases/chart-deps-condition/input/helmfile.yaml new file mode 100644 index 00000000..8e096afb --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/input/helmfile.yaml @@ -0,0 +1,57 @@ +# Test that subcharts 'condition' is working properly + +releases: +# Releases without patches, so not using chartify +- name: testdefault + chart: ./chart + namespace: testdefault +- name: testinstallfalse + chart: ./chart + namespace: testinstallfalse + values: + - subchart1: + install: false +- name: testinstalltrue + chart: ./chart + namespace: testcinstalltrue + values: + - subchart1: + install: true + +# Releases with patches, so using chartify +- name: testchartifydefault + chart: ./chart + namespace: testchartifydefault + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: testsub2 + data: + sub: 42 +- name: testchartifyinstallfalse + chart: ./chart + namespace: testchartifyinstallfalse + values: + - subchart1: + install: false + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: testsub2 + data: + sub: 42 +- name: testchartifyinstalltrue + chart: ./chart + namespace: testchartifyinstalltrue + values: + - subchart1: + install: true + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: testsub2 + data: + sub: 42 diff --git a/test/integration/test-cases/chart-deps-condition/output/template b/test/integration/test-cases/chart-deps-condition/output/template new file mode 100644 index 00000000..4644365f --- /dev/null +++ b/test/integration/test-cases/chart-deps-condition/output/template @@ -0,0 +1,86 @@ +--- +# Source: testchartsdeps/charts/subchart1/templates/cm.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: testsub1 +data: + sub: 1 +--- +# Source: testchartsdeps/charts/subchart2/templates/cm.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: testsub2 +data: + sub: 2 + +--- +# Source: testchartsdeps/charts/subchart2/templates/cm.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: testsub2 +data: + sub: 2 + +--- +# Source: testchartsdeps/charts/subchart1/templates/cm.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: testsub1 +data: + sub: 1 +--- +# Source: testchartsdeps/charts/subchart2/templates/cm.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: testsub2 +data: + sub: 2 + +--- +# Source: testchartsdeps/templates/patched_resources.yaml +apiVersion: v1 +data: + sub: 1 +kind: ConfigMap +metadata: + name: testsub1 +--- +# Source: testchartsdeps/templates/patched_resources.yaml +apiVersion: v1 +data: + sub: 42 +kind: ConfigMap +metadata: + name: testsub2 + +--- +# Source: testchartsdeps/templates/patched_resources.yaml +apiVersion: v1 +data: + sub: 42 +kind: ConfigMap +metadata: + name: testsub2 + +--- +# Source: testchartsdeps/templates/patched_resources.yaml +apiVersion: v1 +data: + sub: 1 +kind: ConfigMap +metadata: + name: testsub1 +--- +# Source: testchartsdeps/templates/patched_resources.yaml +apiVersion: v1 +data: + sub: 42 +kind: ConfigMap +metadata: + name: testsub2 +