Skip to content

Commit

Permalink
Make validation errors easier to fix
Browse files Browse the repository at this point in the history 
Problem: the warnings don't give enough details about which
values are problematic, only the name of the leaf key. This is
all the more annoying when you have a chart depending on other charts.

```
mainchart
 |
 +- subchart1
 +- subchart2
 +- subchart3
```

Here are some warnings I get before the change:
```
coalesce.go:199: warning: destination for credentials is a table. Ignoring non-table value
coalesce.go:160: warning: skipped value for resources: Not a table.
coalesce.go:160: warning: skipped value for googleSheetsServiceAccount: Not a table.
coalesce.go:199: warning: destination for googleSheetsServiceAccount is a table. Ignoring non-table value
coalesce.go:199: warning: destination for resources is a table. Ignoring non-table value []
coalesce.go:199: warning: destination for credentials is a table. Ignoring non-table value
coalesce.go:199: warning: destination for credentials is a table. Ignoring non-table value
coalesce.go:160: warning: skipped value for resources: Not a table.
coalesce.go:160: warning: skipped value for googleSheetsServiceAccount: Not a table.
```

with fix:
```
coalesce.go:162: warning: skipped value for subchart1.resources: Not a table.
coalesce.go:162: warning: skipped value for subchart2.googleSheetsServiceAccount: Not a table.
coalesce.go:211: warning: destination for subchart3.aws.credentials is a table. Ignoring non-table value ()
coalesce.go:211: warning: destination for mainchart.subchart3.aws.credentials is a table. Ignoring non-table value ()
coalesce.go:211: warning: destination for mainchart.subchart2.googleSheetsServiceAccount is a table. Ignoring non-table value ()
coalesce.go:211: warning: destination for mainchart.subchart1.resources is a table. Ignoring non-table value ([])
coalesce.go:162: warning: skipped value for subchart1.resources: Not a table.
coalesce.go:162: warning: skipped value for subchart2.googleSheetsServiceAccount: Not a table.
coalesce.go:211: warning: destination for subchart3.aws.credentials is a table. Ignoring non-table value ()
```

Signed-off-by: Damien Nozay <damiennozay+github@gmail.com>

add tests

Signed-off-by: Damien Nozay <damiennozay+github@gmail.com>
  • Loading branch information
@dnozay @zegerius
dnozay authored and zegerius committed Sep 14, 2021
1 parent adfb52e commit 65ec3d6
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 19 deletions.
59 changes: 40 additions & 19 deletions pkg/chartutil/coalesce.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package chartutil

import (
"fmt"
"log"

"github.com/mitchellh/copystructure"
Expand All @@ -25,6 +26,13 @@ import (
"helm.sh/helm/v3/pkg/chart"
)

func concatPrefix(a, b string) string {
if a == "" {
return b
}
return fmt.Sprintf("%s.%s", a, b)
}

// CoalesceValues coalesces all of the values in a chart (and its subcharts).
//
// Values are coalesced together using the following rules:
Expand All @@ -45,19 +53,21 @@ func CoalesceValues(chrt *chart.Chart, vals map[string]interface{}) (Values, err
if valsCopy == nil {
valsCopy = make(map[string]interface{})
}
return coalesce(chrt, valsCopy)
return coalesce(log.Printf, chrt, valsCopy, "")
}

type printFn func(format string, v ...interface{})

// coalesce coalesces the dest values and the chart values, giving priority to the dest values.
//
// This is a helper function for CoalesceValues.
func coalesce(ch *chart.Chart, dest map[string]interface{}) (map[string]interface{}, error) {
coalesceValues(ch, dest)
return coalesceDeps(ch, dest)
func coalesce(printf printFn, ch *chart.Chart, dest map[string]interface{}, prefix string) (map[string]interface{}, error) {
coalesceValues(printf, ch, dest, prefix)
return coalesceDeps(printf, ch, dest, prefix)
}

// coalesceDeps coalesces the dependencies of the given chart.
func coalesceDeps(chrt *chart.Chart, dest map[string]interface{}) (map[string]interface{}, error) {
func coalesceDeps(printf printFn, chrt *chart.Chart, dest map[string]interface{}, prefix string) (map[string]interface{}, error) {
for _, subchart := range chrt.Dependencies() {
if c, ok := dest[subchart.Name()]; !ok {
// If dest doesn't already have the key, create it.
Expand All @@ -67,13 +77,14 @@ func coalesceDeps(chrt *chart.Chart, dest map[string]interface{}) (map[string]in
}
if dv, ok := dest[subchart.Name()]; ok {
dvmap := dv.(map[string]interface{})
subPrefix := concatPrefix(prefix, chrt.Metadata.Name)

// Get globals out of dest and merge them into dvmap.
coalesceGlobals(dvmap, dest)
coalesceGlobals(printf, dvmap, dest, subPrefix)

// Now coalesce the rest of the values.
var err error
dest[subchart.Name()], err = coalesce(subchart, dvmap)
dest[subchart.Name()], err = coalesce(printf, subchart, dvmap, subPrefix)
if err != nil {
return dest, err
}
Expand All @@ -85,20 +96,20 @@ func coalesceDeps(chrt *chart.Chart, dest map[string]interface{}) (map[string]in
// coalesceGlobals copies the globals out of src and merges them into dest.
//
// For convenience, returns dest.
func coalesceGlobals(dest, src map[string]interface{}) {
func coalesceGlobals(printf printFn, dest, src map[string]interface{}, prefix string) {
var dg, sg map[string]interface{}

if destglob, ok := dest[GlobalKey]; !ok {
dg = make(map[string]interface{})
} else if dg, ok = destglob.(map[string]interface{}); !ok {
log.Printf("warning: skipping globals because destination %s is not a table.", GlobalKey)
printf("warning: skipping globals because destination %s is not a table.", GlobalKey)
return
}

if srcglob, ok := src[GlobalKey]; !ok {
sg = make(map[string]interface{})
} else if sg, ok = srcglob.(map[string]interface{}); !ok {
log.Printf("warning: skipping globals because source %s is not a table.", GlobalKey)
printf("warning: skipping globals because source %s is not a table.", GlobalKey)
return
}

Expand All @@ -114,17 +125,18 @@ func coalesceGlobals(dest, src map[string]interface{}) {
dg[key] = vv
} else {
if destvmap, ok := destv.(map[string]interface{}); !ok {
log.Printf("Conflict: cannot merge map onto non-map for %q. Skipping.", key)
printf("Conflict: cannot merge map onto non-map for %q. Skipping.", key)
} else {
// Basically, we reverse order of coalesce here to merge
// top-down.
CoalesceTables(vv, destvmap)
subPrefix := concatPrefix(prefix, key)
coalesceTablesFullKey(printf, vv, destvmap, subPrefix)
dg[key] = vv
}
}
} else if dv, ok := dg[key]; ok && istable(dv) {
// It's not clear if this condition can actually ever trigger.
log.Printf("key %s is table. Skipping", key)
printf("key %s is table. Skipping", key)
} else {
// TODO: Do we need to do any additional checking on the value?
dg[key] = val
Expand All @@ -144,7 +156,8 @@ func copyMap(src map[string]interface{}) map[string]interface{} {
// coalesceValues builds up a values map for a particular chart.
//
// Values in v will override the values in the chart.
func coalesceValues(c *chart.Chart, v map[string]interface{}) {
func coalesceValues(printf printFn, c *chart.Chart, v map[string]interface{}, prefix string) {
subPrefix := concatPrefix(prefix, c.Metadata.Name)
for key, val := range c.Values {
if value, ok := v[key]; ok {
if value == nil {
Expand All @@ -159,12 +172,12 @@ func coalesceValues(c *chart.Chart, v map[string]interface{}) {
// If the original value is nil, there is nothing to coalesce, so we don't print
// the warning
if val != nil {
log.Printf("warning: skipped value for %s: Not a table.", key)
printf("warning: skipped value for %s.%s: Not a table.", subPrefix, key)
}
} else {
// Because v has higher precedence than nv, dest values override src
// values.
CoalesceTables(dest, src)
coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key))
}
}
} else {
Expand All @@ -178,6 +191,13 @@ func coalesceValues(c *chart.Chart, v map[string]interface{}) {
//
// dest is considered authoritative.
func CoalesceTables(dst, src map[string]interface{}) map[string]interface{} {
return coalesceTablesFullKey(log.Printf, dst, src, "")
}

// coalesceTablesFullKey merges a source map into a destination map.
//
// dest is considered authoritative.
func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, prefix string) map[string]interface{} {
// When --reuse-values is set but there are no modifications yet, return new values
if src == nil {
return dst
Expand All @@ -188,18 +208,19 @@ func CoalesceTables(dst, src map[string]interface{}) map[string]interface{} {
// Because dest has higher precedence than src, dest values override src
// values.
for key, val := range src {
fullkey := concatPrefix(prefix, key)
if dv, ok := dst[key]; ok && dv == nil {
delete(dst, key)
} else if !ok {
dst[key] = val
} else if istable(val) {
if istable(dv) {
CoalesceTables(dv.(map[string]interface{}), val.(map[string]interface{}))
coalesceTablesFullKey(printf, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey)
} else {
log.Printf("warning: cannot overwrite table with non table for %s (%v)", key, val)
printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val)
}
} else if istable(dv) && val != nil {
log.Printf("warning: destination for %s is a table. Ignoring non-table value %v", key, val)
printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val)
}
}
return dst
Expand Down
68 changes: 68 additions & 0 deletions pkg/chartutil/coalesce_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package chartutil

import (
"encoding/json"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -339,3 +340,70 @@ func TestCoalesceTables(t *testing.T) {
t.Errorf("Expected hole string, got %v", dst2["boat"])
}
}

func TestCoalesceValuesWarnings(t *testing.T) {

c := withDeps(&chart.Chart{
Metadata: &chart.Metadata{Name: "level1"},
Values: map[string]interface{}{
"name": "moby",
},
},
withDeps(&chart.Chart{
Metadata: &chart.Metadata{Name: "level2"},
Values: map[string]interface{}{
"name": "pequod",
},
},
&chart.Chart{
Metadata: &chart.Metadata{Name: "level3"},
Values: map[string]interface{}{
"name": "ahab",
"boat": true,
"spear": map[string]interface{}{
"tip": true,
"sail": map[string]interface{}{
"cotton": true,
},
},
},
},
),
)

vals := map[string]interface{}{
"level2": map[string]interface{}{
"level3": map[string]interface{}{
"boat": map[string]interface{}{"mast": true},
"spear": map[string]interface{}{
"tip": map[string]interface{}{
"sharp": true,
},
"sail": true,
},
},
},
}

warnings := make([]string, 0)
printf := func(format string, v ...interface{}) {
t.Logf(format, v...)
warnings = append(warnings, fmt.Sprintf(format, v...))
}

_, err := coalesce(printf, c, vals, "")
if err != nil {
t.Fatal(err)
}

t.Logf("vals: %v", vals)
assert.Contains(t, warnings, "warning: skipped value for level1.level2.level3.boat: Not a table.")
assert.Contains(t, warnings, "warning: destination for level1.level2.level3.spear.tip is a table. Ignoring non-table value (true)")
assert.Contains(t, warnings, "warning: cannot overwrite table with non table for level1.level2.level3.spear.sail (map[cotton:true])")

}

func TestConcatPrefix(t *testing.T) {
assert.Equal(t, "b", concatPrefix("", "b"))
assert.Equal(t, "a.b", concatPrefix("a", "b"))
}

0 comments on commit 65ec3d6

Please sign in to comment.