From 0d36cb664a30826ad3f1b811e5ad942144e85336 Mon Sep 17 00:00:00 2001 From: lubingtan Date: Fri, 24 Jan 2025 16:21:02 +0800 Subject: [PATCH 1/6] feat: support multi-document values files Signed-off-by: lubingtan --- pkg/cli/values/options.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/pkg/cli/values/options.go b/pkg/cli/values/options.go index add2f72d5..e1f1988c8 100644 --- a/pkg/cli/values/options.go +++ b/pkg/cli/values/options.go @@ -17,6 +17,7 @@ limitations under the License. package values import ( + "bytes" "encoding/json" "io" "net/url" @@ -24,7 +25,7 @@ import ( "strings" "github.com/pkg/errors" - "sigs.k8s.io/yaml" + utilyaml "k8s.io/apimachinery/pkg/util/yaml" "helm.sh/helm/v4/pkg/getter" "helm.sh/helm/v4/pkg/strvals" @@ -47,18 +48,23 @@ func (opts *Options) MergeValues(p getter.Providers) (map[string]interface{}, er // User specified a values files via -f/--values for _, filePath := range opts.ValueFiles { - currentMap := map[string]interface{}{} - - bytes, err := readFile(filePath, p) + raw, err := readFile(filePath, p) if err != nil { return nil, err } - if err := yaml.Unmarshal(bytes, ¤tMap); err != nil { - return nil, errors.Wrapf(err, "failed to parse %s", filePath) + decoder := utilyaml.NewYAMLOrJSONDecoder(bytes.NewReader(raw), 4096) + for { + currentMap := map[string]interface{}{} + if err := decoder.Decode(¤tMap); err != nil { + if err == io.EOF { + break + } + return nil, errors.Wrapf(err, "failed to parse %s", filePath) + } + // Merge with the previous map + base = mergeMaps(base, currentMap) } - // Merge with the previous map - base = mergeMaps(base, currentMap) } // User specified a value via --set-json From 92087f6e33d6a5d8333e6afda7d68a4d15a2cec9 Mon Sep 17 00:00:00 2001 From: lubingtan Date: Mon, 3 Feb 2025 20:53:07 +0800 Subject: [PATCH 2/6] feat: support multi-document values files for default chart values Signed-off-by: lubingtan --- pkg/chart/loader/load.go | 53 ++++++++++++++-- pkg/chart/loader/load_test.go | 113 +++++++++++++++++++++++++++++++++ pkg/cli/values/options.go | 41 ++---------- pkg/cli/values/options_test.go | 55 ---------------- 4 files changed, 168 insertions(+), 94 deletions(-) diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go index 7645ba96c..c40a1aaf5 100644 --- a/pkg/chart/loader/load.go +++ b/pkg/chart/loader/load.go @@ -17,14 +17,17 @@ limitations under the License. package loader import ( + "bufio" "bytes" "encoding/json" + "io" "log" "os" "path/filepath" "strings" "github.com/pkg/errors" + utilyaml "k8s.io/apimachinery/pkg/util/yaml" "sigs.k8s.io/yaml" "helm.sh/helm/v4/pkg/chart" @@ -104,13 +107,11 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { return c, errors.Wrap(err, "cannot load Chart.lock") } case f.Name == "values.yaml": - c.Values = make(map[string]interface{}) - if err := yaml.Unmarshal(f.Data, &c.Values, func(d *json.Decoder) *json.Decoder { - d.UseNumber() - return d - }); err != nil { + values, err := LoadValues(f.Data) + if err != nil { return c, errors.Wrap(err, "cannot load values.yaml") } + c.Values = values case f.Name == "values.schema.json": c.Schema = f.Data @@ -205,3 +206,45 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { return c, nil } + +func LoadValues(data []byte) (map[string]interface{}, error) { + values := map[string]interface{}{} + reader := utilyaml.NewYAMLReader(bufio.NewReader(bytes.NewReader(data))) + for { + currentMap := map[string]interface{}{} + raw, err := reader.Read() + if err != nil { + if err == io.EOF { + break + } + return nil, errors.Wrap(err, "error reading yaml document") + } + if err := yaml.Unmarshal(raw, ¤tMap, func(d *json.Decoder) *json.Decoder { + d.UseNumber() + return d + }); err != nil { + return nil, errors.Wrap(err, "cannot unmarshal yaml document") + } + values = MergeMaps(values, currentMap) + } + return values, nil +} + +func MergeMaps(a, b map[string]interface{}) map[string]interface{} { + out := make(map[string]interface{}, len(a)) + for k, v := range a { + out[k] = v + } + for k, v := range b { + if v, ok := v.(map[string]interface{}); ok { + if bv, ok := out[k]; ok { + if bv, ok := bv.(map[string]interface{}); ok { + out[k] = MergeMaps(bv, v) + continue + } + } + } + out[k] = v + } + return out +} diff --git a/pkg/chart/loader/load_test.go b/pkg/chart/loader/load_test.go index 25e000835..92ffa002b 100644 --- a/pkg/chart/loader/load_test.go +++ b/pkg/chart/loader/load_test.go @@ -24,6 +24,7 @@ import ( "log" "os" "path/filepath" + "reflect" "runtime" "strings" "testing" @@ -488,6 +489,118 @@ func TestLoadInvalidArchive(t *testing.T) { } } +func TestLoadValues(t *testing.T) { + testDatas := []struct { + name string + data []byte + expctedValues map[string]interface{} + }{ + { + name: "It should load values correctly", + data: []byte(` +foo: + image: foo:v1 +bar: + version: v2 +`), + expctedValues: map[string]interface{}{ + "foo": map[string]interface{}{ + "image": "foo:v1", + }, + "bar": map[string]interface{}{ + "version": "v2", + }, + }, + }, + { + name: "It should load values correctly with multiple documents in one file", + data: []byte(` +foo: + image: foo:v1 +bar: + version: v2 +--- +foo: + image: foo:v2 +`), + expctedValues: map[string]interface{}{ + "foo": map[string]interface{}{ + "image": "foo:v2", + }, + "bar": map[string]interface{}{ + "version": "v2", + }, + }, + }, + } + for _, testData := range testDatas { + t.Run(testData.name, func(tt *testing.T) { + values, err := LoadValues(testData.data) + if err != nil { + tt.Fatal(err) + } + if !reflect.DeepEqual(values, testData.expctedValues) { + tt.Errorf("Expected values: %v, got %v", testData.expctedValues, values) + } + }) + } +} + +func TestMergeValues(t *testing.T) { + nestedMap := map[string]interface{}{ + "foo": "bar", + "baz": map[string]string{ + "cool": "stuff", + }, + } + anotherNestedMap := map[string]interface{}{ + "foo": "bar", + "baz": map[string]string{ + "cool": "things", + "awesome": "stuff", + }, + } + flatMap := map[string]interface{}{ + "foo": "bar", + "baz": "stuff", + } + anotherFlatMap := map[string]interface{}{ + "testing": "fun", + } + + testMap := MergeMaps(flatMap, nestedMap) + equal := reflect.DeepEqual(testMap, nestedMap) + if !equal { + t.Errorf("Expected a nested map to overwrite a flat value. Expected: %v, got %v", nestedMap, testMap) + } + + testMap = MergeMaps(nestedMap, flatMap) + equal = reflect.DeepEqual(testMap, flatMap) + if !equal { + t.Errorf("Expected a flat value to overwrite a map. Expected: %v, got %v", flatMap, testMap) + } + + testMap = MergeMaps(nestedMap, anotherNestedMap) + equal = reflect.DeepEqual(testMap, anotherNestedMap) + if !equal { + t.Errorf("Expected a nested map to overwrite another nested map. Expected: %v, got %v", anotherNestedMap, testMap) + } + + testMap = MergeMaps(anotherFlatMap, anotherNestedMap) + expectedMap := map[string]interface{}{ + "testing": "fun", + "foo": "bar", + "baz": map[string]string{ + "cool": "things", + "awesome": "stuff", + }, + } + equal = reflect.DeepEqual(testMap, expectedMap) + if !equal { + t.Errorf("Expected a map with different keys to merge properly with another map. Expected: %v, got %v", expectedMap, testMap) + } +} + func verifyChart(t *testing.T, c *chart.Chart) { t.Helper() if c.Name() == "" { diff --git a/pkg/cli/values/options.go b/pkg/cli/values/options.go index e1f1988c8..70390f12a 100644 --- a/pkg/cli/values/options.go +++ b/pkg/cli/values/options.go @@ -17,7 +17,6 @@ limitations under the License. package values import ( - "bytes" "encoding/json" "io" "net/url" @@ -25,8 +24,8 @@ import ( "strings" "github.com/pkg/errors" - utilyaml "k8s.io/apimachinery/pkg/util/yaml" + "helm.sh/helm/v4/pkg/chart/loader" "helm.sh/helm/v4/pkg/getter" "helm.sh/helm/v4/pkg/strvals" ) @@ -48,23 +47,16 @@ func (opts *Options) MergeValues(p getter.Providers) (map[string]interface{}, er // User specified a values files via -f/--values for _, filePath := range opts.ValueFiles { - raw, err := readFile(filePath, p) + bytes, err := readFile(filePath, p) if err != nil { return nil, err } - - decoder := utilyaml.NewYAMLOrJSONDecoder(bytes.NewReader(raw), 4096) - for { - currentMap := map[string]interface{}{} - if err := decoder.Decode(¤tMap); err != nil { - if err == io.EOF { - break - } - return nil, errors.Wrapf(err, "failed to parse %s", filePath) - } - // Merge with the previous map - base = mergeMaps(base, currentMap) + currentMap, err := loader.LoadValues(bytes) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse %s", filePath) } + // Merge with the previous map + base = loader.MergeMaps(base, currentMap) } // User specified a value via --set-json @@ -123,25 +115,6 @@ func (opts *Options) MergeValues(p getter.Providers) (map[string]interface{}, er return base, nil } -func mergeMaps(a, b map[string]interface{}) map[string]interface{} { - out := make(map[string]interface{}, len(a)) - for k, v := range a { - out[k] = v - } - for k, v := range b { - if v, ok := v.(map[string]interface{}); ok { - if bv, ok := out[k]; ok { - if bv, ok := bv.(map[string]interface{}); ok { - out[k] = mergeMaps(bv, v) - continue - } - } - } - out[k] = v - } - return out -} - // readFile load a file from stdin, the local directory, or a remote file with a url. func readFile(filePath string, p getter.Providers) ([]byte, error) { if strings.TrimSpace(filePath) == "-" { diff --git a/pkg/cli/values/options_test.go b/pkg/cli/values/options_test.go index 5197a1b5e..c3bb0af33 100644 --- a/pkg/cli/values/options_test.go +++ b/pkg/cli/values/options_test.go @@ -23,61 +23,6 @@ import ( "helm.sh/helm/v4/pkg/getter" ) -func TestMergeMaps(t *testing.T) { - nestedMap := map[string]interface{}{ - "foo": "bar", - "baz": map[string]string{ - "cool": "stuff", - }, - } - anotherNestedMap := map[string]interface{}{ - "foo": "bar", - "baz": map[string]string{ - "cool": "things", - "awesome": "stuff", - }, - } - flatMap := map[string]interface{}{ - "foo": "bar", - "baz": "stuff", - } - anotherFlatMap := map[string]interface{}{ - "testing": "fun", - } - - testMap := mergeMaps(flatMap, nestedMap) - equal := reflect.DeepEqual(testMap, nestedMap) - if !equal { - t.Errorf("Expected a nested map to overwrite a flat value. Expected: %v, got %v", nestedMap, testMap) - } - - testMap = mergeMaps(nestedMap, flatMap) - equal = reflect.DeepEqual(testMap, flatMap) - if !equal { - t.Errorf("Expected a flat value to overwrite a map. Expected: %v, got %v", flatMap, testMap) - } - - testMap = mergeMaps(nestedMap, anotherNestedMap) - equal = reflect.DeepEqual(testMap, anotherNestedMap) - if !equal { - t.Errorf("Expected a nested map to overwrite another nested map. Expected: %v, got %v", anotherNestedMap, testMap) - } - - testMap = mergeMaps(anotherFlatMap, anotherNestedMap) - expectedMap := map[string]interface{}{ - "testing": "fun", - "foo": "bar", - "baz": map[string]string{ - "cool": "things", - "awesome": "stuff", - }, - } - equal = reflect.DeepEqual(testMap, expectedMap) - if !equal { - t.Errorf("Expected a map with different keys to merge properly with another map. Expected: %v, got %v", expectedMap, testMap) - } -} - func TestReadFile(t *testing.T) { var p getter.Providers filePath := "%a.txt" From 3d84e00ce756f34f8d3cf41c1489548696edd8a7 Mon Sep 17 00:00:00 2001 From: lubingtan Date: Sun, 9 Feb 2025 16:16:08 +0800 Subject: [PATCH 3/6] fix: use Reader interface as the input of LoadValues and enhance UT of LoadValues Signed-off-by: lubingtan --- pkg/chart/loader/load.go | 7 ++++--- pkg/chart/loader/load_test.go | 19 ++++++++----------- pkg/cli/values/options.go | 5 +++-- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go index c40a1aaf5..84a040c0f 100644 --- a/pkg/chart/loader/load.go +++ b/pkg/chart/loader/load.go @@ -107,7 +107,7 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { return c, errors.Wrap(err, "cannot load Chart.lock") } case f.Name == "values.yaml": - values, err := LoadValues(f.Data) + values, err := LoadValues(bytes.NewReader(f.Data)) if err != nil { return c, errors.Wrap(err, "cannot load values.yaml") } @@ -207,9 +207,10 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { return c, nil } -func LoadValues(data []byte) (map[string]interface{}, error) { +// LoadValues loads chat values from a reader. +func LoadValues(data io.Reader) (map[string]interface{}, error) { values := map[string]interface{}{} - reader := utilyaml.NewYAMLReader(bufio.NewReader(bytes.NewReader(data))) + reader := utilyaml.NewYAMLReader(bufio.NewReader(data)) for { currentMap := map[string]interface{}{} raw, err := reader.Read() diff --git a/pkg/chart/loader/load_test.go b/pkg/chart/loader/load_test.go index 92ffa002b..e34124829 100644 --- a/pkg/chart/loader/load_test.go +++ b/pkg/chart/loader/load_test.go @@ -490,13 +490,11 @@ func TestLoadInvalidArchive(t *testing.T) { } func TestLoadValues(t *testing.T) { - testDatas := []struct { - name string + testCases := map[string]struct { data []byte expctedValues map[string]interface{} }{ - { - name: "It should load values correctly", + "It should load values correctly": { data: []byte(` foo: image: foo:v1 @@ -512,8 +510,7 @@ bar: }, }, }, - { - name: "It should load values correctly with multiple documents in one file", + "It should load values correctly with multiple documents in one file": { data: []byte(` foo: image: foo:v1 @@ -533,14 +530,14 @@ foo: }, }, } - for _, testData := range testDatas { - t.Run(testData.name, func(tt *testing.T) { - values, err := LoadValues(testData.data) + for testName, testCase := range testCases { + t.Run(testName, func(tt *testing.T) { + values, err := LoadValues(bytes.NewReader(testCase.data)) if err != nil { tt.Fatal(err) } - if !reflect.DeepEqual(values, testData.expctedValues) { - tt.Errorf("Expected values: %v, got %v", testData.expctedValues, values) + if !reflect.DeepEqual(values, testCase.expctedValues) { + tt.Errorf("Expected values: %v, got %v", testCase.expctedValues, values) } }) } diff --git a/pkg/cli/values/options.go b/pkg/cli/values/options.go index 70390f12a..54dd288bc 100644 --- a/pkg/cli/values/options.go +++ b/pkg/cli/values/options.go @@ -17,6 +17,7 @@ limitations under the License. package values import ( + "bytes" "encoding/json" "io" "net/url" @@ -47,11 +48,11 @@ func (opts *Options) MergeValues(p getter.Providers) (map[string]interface{}, er // User specified a values files via -f/--values for _, filePath := range opts.ValueFiles { - bytes, err := readFile(filePath, p) + raw, err := readFile(filePath, p) if err != nil { return nil, err } - currentMap, err := loader.LoadValues(bytes) + currentMap, err := loader.LoadValues(bytes.NewReader(raw)) if err != nil { return nil, errors.Wrapf(err, "failed to parse %s", filePath) } From fb7221bc9aa8f7747d7f31bffa1181886b7de6ec Mon Sep 17 00:00:00 2001 From: lubingtan Date: Sun, 9 Feb 2025 22:13:26 +0800 Subject: [PATCH 4/6] fix: add doc for func MergeMaps Signed-off-by: lubingtan --- pkg/chart/loader/load.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go index 84a040c0f..c957c398d 100644 --- a/pkg/chart/loader/load.go +++ b/pkg/chart/loader/load.go @@ -231,6 +231,8 @@ func LoadValues(data io.Reader) (map[string]interface{}, error) { return values, nil } +// MergeMaps merges two maps. If a key exists in both maps, the value from b will be used. +// If the value is a map, the maps will be merged recursively. func MergeMaps(a, b map[string]interface{}) map[string]interface{} { out := make(map[string]interface{}, len(a)) for k, v := range a { From 91cd72d0e47cd5a45871037894927c62956b0f98 Mon Sep 17 00:00:00 2001 From: lubingtan Date: Mon, 10 Feb 2025 08:25:23 +0800 Subject: [PATCH 5/6] fix: improve LoadValues function documentation Signed-off-by: lubingtan --- pkg/chart/loader/load.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go index c957c398d..e32094ef5 100644 --- a/pkg/chart/loader/load.go +++ b/pkg/chart/loader/load.go @@ -207,7 +207,10 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { return c, nil } -// LoadValues loads chat values from a reader. +// LoadValues loads values from a reader. +// +// The reader is expected to contain one or more YAML documents, the values of which are merged. +// And the values can be either a chart's default values or a user-supplied values. func LoadValues(data io.Reader) (map[string]interface{}, error) { values := map[string]interface{}{} reader := utilyaml.NewYAMLReader(bufio.NewReader(data)) From ef5614364bb174df3126fbb7d0d89b39c2af5c8f Mon Sep 17 00:00:00 2001 From: lubingtan Date: Thu, 20 Feb 2025 08:13:54 +0800 Subject: [PATCH 6/6] fix: replace mergeMaps call with loader.MergeMaps in MergeValues function Signed-off-by: lubingtan --- pkg/cli/values/options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cli/values/options.go b/pkg/cli/values/options.go index 54dd288bc..2c3706b84 100644 --- a/pkg/cli/values/options.go +++ b/pkg/cli/values/options.go @@ -69,7 +69,7 @@ func (opts *Options) MergeValues(p getter.Providers) (map[string]interface{}, er if err := json.Unmarshal([]byte(trimmedValue), &jsonMap); err != nil { return nil, errors.Errorf("failed parsing --set-json data JSON: %s", value) } - base = mergeMaps(base, jsonMap) + base = loader.MergeMaps(base, jsonMap) } else { // Otherwise, parse it as key=value format if err := strvals.ParseJSON(value, base); err != nil {