From 58c8aca1ccf20a350635172a76e0262057e88992 Mon Sep 17 00:00:00 2001 From: Justin Scott Date: Thu, 9 Feb 2017 14:58:35 -0800 Subject: [PATCH] feat(helm): fixup if/ele,remove extra string casts, add comments --- pkg/chartutil/requirements.go | 119 ++++++++++-------- pkg/chartutil/requirements_test.go | 22 ++-- pkg/chartutil/testdata/subpop/Chart.yaml | 2 +- .../subpop/charts/subchart1/requirements.yaml | 2 +- .../subpop/charts/subchart2/requirements.yaml | 2 +- .../testdata/subpop/noreqs/Chart.yaml | 2 +- .../testdata/subpop/noreqs/values.yaml | 1 - .../testdata/subpop/requirements.yaml | 2 +- pkg/chartutil/values.go | 12 +- 9 files changed, 88 insertions(+), 76 deletions(-) diff --git a/pkg/chartutil/requirements.go b/pkg/chartutil/requirements.go index 336d0ba61..3a31042d6 100644 --- a/pkg/chartutil/requirements.go +++ b/pkg/chartutil/requirements.go @@ -121,52 +121,55 @@ func LoadRequirementsLock(c *chart.Chart) (*RequirementsLock, error) { func ProcessRequirementsConditions(reqs *Requirements, cvals Values) { var cond string var conds []string - - if reqs != nil && len(reqs.Dependencies) > 0 { - for _, r := range reqs.Dependencies { - var hasTrue, hasFalse bool - cond = string(r.Condition) - // check for list - if len(cond) > 0 { - if strings.Contains(cond, ",") { - conds = strings.Split(strings.TrimSpace(cond), ",") - } else { - conds = []string{strings.TrimSpace(cond)} - } - for _, c := range conds { - if len(c) > 0 { - // retrieve value - vv, err := cvals.PathValue(c) - if err == nil { - if vv.(bool) { + if reqs == nil || len(reqs.Dependencies) == 0 { + return + } + for _, r := range reqs.Dependencies { + var hasTrue, hasFalse bool + cond = string(r.Condition) + // check for list + if len(cond) > 0 { + if strings.Contains(cond, ",") { + conds = strings.Split(strings.TrimSpace(cond), ",") + } else { + conds = []string{strings.TrimSpace(cond)} + } + for _, c := range conds { + if len(c) > 0 { + // retrieve value + vv, err := cvals.PathValue(c) + if err == nil { + // if not bool, warn + if bv, ok := vv.(bool); ok { + if bv { hasTrue = true - } - if !vv.(bool) { + } else { hasFalse = true } } else { - if _, ok := err.(ErrNoValue); !ok { - // this is a real error - log.Printf("Warning: PathValue returned error %v", err) - } - } - if vv != nil { - // got first value, break loop - break + log.Printf("Warning: Condition path '%s' for chart %s returned non-bool value", c, r.Name) } + } else if _, ok := err.(ErrNoValue); !ok { + // this is a real error + log.Printf("Warning: PathValue returned error %v", err) + } - } - if !hasTrue && hasFalse { - r.Enabled = false - } else { - if hasTrue { - r.Enabled = true + if vv != nil { + // got first value, break loop + break } } } + if !hasTrue && hasFalse { + r.Enabled = false + } else if hasTrue { + r.Enabled = true + } } + } + } // ProcessRequirementsTags disables charts based on tags in values @@ -176,33 +179,38 @@ func ProcessRequirementsTags(reqs *Requirements, cvals Values) { return } - if reqs != nil && len(reqs.Dependencies) > 0 { - for _, r := range reqs.Dependencies { - if len(r.Tags) > 0 { - tags := r.Tags + if reqs == nil || len(reqs.Dependencies) == 0 { + return + } + for _, r := range reqs.Dependencies { + if len(r.Tags) > 0 { + tags := r.Tags - var hasTrue, hasFalse bool - for _, k := range tags { - if b, ok := vt[k]; ok { - if b.(bool) { + var hasTrue, hasFalse bool + for _, k := range tags { + if b, ok := vt[k]; ok { + // if not bool, warn + if bv, ok := b.(bool); ok { + if bv { hasTrue = true - } - if !b.(bool) { + } else { hasFalse = true } + } else { + log.Printf("Warning: Tag '%s' for chart %s returned non-bool value", k, r.Name) } } - if !hasTrue && hasFalse { - r.Enabled = false - } else { - if hasTrue || !hasTrue && !hasFalse { - r.Enabled = true - } - } + } + if !hasTrue && hasFalse { + r.Enabled = false + } else if hasTrue || !hasTrue && !hasFalse { + r.Enabled = true } + } } + } // ProcessRequirementsEnabled removes disabled charts from dependencies @@ -212,10 +220,10 @@ func ProcessRequirementsEnabled(c *chart.Chart, v *chart.Config) error { // if not just missing requirements file, return error if nerr, ok := err.(ErrNoRequirementsFile); !ok { return nerr - } else { - // no requirements to process - return nil } + + // no requirements to process + return nil } // set all to true for _, lr := range reqs.Dependencies { @@ -238,7 +246,8 @@ func ProcessRequirementsEnabled(c *chart.Chart, v *chart.Config) error { } } // don't keep disabled charts in new slice - cd := c.Dependencies[:0] + cd := []*chart.Chart{} + copy(cd, c.Dependencies[:0]) for _, n := range c.Dependencies { if _, ok := rm[n.Metadata.Name]; !ok { cd = append(cd, n) diff --git a/pkg/chartutil/requirements_test.go b/pkg/chartutil/requirements_test.go index ff3e895df..b9a5ae12a 100644 --- a/pkg/chartutil/requirements_test.go +++ b/pkg/chartutil/requirements_test.go @@ -42,7 +42,7 @@ func TestRequirementsTagsNonValue(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags with no effect - v := &chart.Config{Raw: string("tags:\n nothinguseful: false\n\n")} + v := &chart.Config{Raw: "tags:\n nothinguseful: false\n\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart1", "subcharta", "subchartb"} @@ -54,7 +54,7 @@ func TestRequirementsTagsDisabledL1(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags disabling a group - v := &chart.Config{Raw: string("tags:\n front-end: false\n\n")} + v := &chart.Config{Raw: "tags:\n front-end: false\n\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart"} @@ -66,7 +66,7 @@ func TestRequirementsTagsEnabledL1(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags disabling a group and enabling a different group - v := &chart.Config{Raw: string("tags:\n front-end: false\n\n back-end: true\n")} + v := &chart.Config{Raw: "tags:\n front-end: false\n\n back-end: true\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart2", "subchartb", "subchartc"} @@ -78,7 +78,7 @@ func TestRequirementsTagsDisabledL2(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags disabling only children - v := &chart.Config{Raw: string("tags:\n subcharta: false\n\n subchartb: false\n")} + v := &chart.Config{Raw: "tags:\n subcharta: false\n\n subchartb: false\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart1"} @@ -90,7 +90,7 @@ func TestRequirementsTagsDisabledL1Mixed(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags disabling all parents/children with additional tag re-enabling a parent - v := &chart.Config{Raw: string("tags:\n front-end: false\n\n subchart1: true\n\n back-end: false\n")} + v := &chart.Config{Raw: "tags:\n front-end: false\n\n subchart1: true\n\n back-end: false\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart1"} @@ -102,7 +102,7 @@ func TestRequirementsConditionsNonValue(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags with no effect - v := &chart.Config{Raw: string("subchart1:\n nothinguseful: false\n\n")} + v := &chart.Config{Raw: "subchart1:\n nothinguseful: false\n\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart1", "subcharta", "subchartb"} @@ -114,7 +114,7 @@ func TestRequirementsConditionsEnabledL1Both(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // conditions enabling the parent charts, effectively enabling children - v := &chart.Config{Raw: string("subchart1:\n enabled: true\nsubchart2:\n enabled: true\n")} + v := &chart.Config{Raw: "subchart1:\n enabled: true\nsubchart2:\n enabled: true\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart1", "subchart2", "subcharta", "subchartb", "subchartb", "subchartc"} @@ -126,7 +126,7 @@ func TestRequirementsConditionsDisabledL1Both(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // conditions disabling the parent charts, effectively disabling children - v := &chart.Config{Raw: string("subchart1:\n enabled: false\nsubchart2:\n enabled: false\n")} + v := &chart.Config{Raw: "subchart1:\n enabled: false\nsubchart2:\n enabled: false\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart"} @@ -139,7 +139,7 @@ func TestRequirementsConditionsSecond(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // conditions a child using the second condition path of child's condition - v := &chart.Config{Raw: string("subchart1:\n subcharta:\n enabled: false\n")} + v := &chart.Config{Raw: "subchart1:\n subcharta:\n enabled: false\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart1", "subchartb"} @@ -151,7 +151,7 @@ func TestRequirementsCombinedDisabledL2(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags enabling a parent/child group with condition disabling one child - v := &chart.Config{Raw: string("subchartc:\n enabled: false\ntags:\n back-end: true\n")} + v := &chart.Config{Raw: "subchartc:\n enabled: false\ntags:\n back-end: true\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart", "subchart1", "subchart2", "subcharta", "subchartb", "subchartb"} @@ -163,7 +163,7 @@ func TestRequirementsCombinedDisabledL1(t *testing.T) { t.Fatalf("Failed to load testdata: %s", err) } // tags will not enable a child if parent is explicitly disabled with condition - v := &chart.Config{Raw: string("subchart1:\n enabled: false\ntags:\n front-end: true\n")} + v := &chart.Config{Raw: "subchart1:\n enabled: false\ntags:\n front-end: true\n"} // expected charts including duplicates in alphanumeric order e := []string{"parentchart"} diff --git a/pkg/chartutil/testdata/subpop/Chart.yaml b/pkg/chartutil/testdata/subpop/Chart.yaml index 77bda03b6..bbb0941c3 100644 --- a/pkg/chartutil/testdata/subpop/Chart.yaml +++ b/pkg/chartutil/testdata/subpop/Chart.yaml @@ -1,4 +1,4 @@ apiVersion: v1 description: A Helm chart for Kubernetes name: parentchart -version: 0.1.0 \ No newline at end of file +version: 0.1.0 diff --git a/pkg/chartutil/testdata/subpop/charts/subchart1/requirements.yaml b/pkg/chartutil/testdata/subpop/charts/subchart1/requirements.yaml index 3fa0b8dfb..94d278234 100644 --- a/pkg/chartutil/testdata/subpop/charts/subchart1/requirements.yaml +++ b/pkg/chartutil/testdata/subpop/charts/subchart1/requirements.yaml @@ -12,4 +12,4 @@ dependencies: condition: subchartb.enabled tags: - front-end - - subchartb \ No newline at end of file + - subchartb diff --git a/pkg/chartutil/testdata/subpop/charts/subchart2/requirements.yaml b/pkg/chartutil/testdata/subpop/charts/subchart2/requirements.yaml index b68d1a776..1f0023a08 100644 --- a/pkg/chartutil/testdata/subpop/charts/subchart2/requirements.yaml +++ b/pkg/chartutil/testdata/subpop/charts/subchart2/requirements.yaml @@ -12,4 +12,4 @@ dependencies: condition: subchartc.enabled tags: - back-end - - subchartc \ No newline at end of file + - subchartc diff --git a/pkg/chartutil/testdata/subpop/noreqs/Chart.yaml b/pkg/chartutil/testdata/subpop/noreqs/Chart.yaml index 77bda03b6..bbb0941c3 100644 --- a/pkg/chartutil/testdata/subpop/noreqs/Chart.yaml +++ b/pkg/chartutil/testdata/subpop/noreqs/Chart.yaml @@ -1,4 +1,4 @@ apiVersion: v1 description: A Helm chart for Kubernetes name: parentchart -version: 0.1.0 \ No newline at end of file +version: 0.1.0 diff --git a/pkg/chartutil/testdata/subpop/noreqs/values.yaml b/pkg/chartutil/testdata/subpop/noreqs/values.yaml index 81c18c6b0..4ed3b7ad3 100644 --- a/pkg/chartutil/testdata/subpop/noreqs/values.yaml +++ b/pkg/chartutil/testdata/subpop/noreqs/values.yaml @@ -24,4 +24,3 @@ resources: tags: front-end: true back-end: false - diff --git a/pkg/chartutil/testdata/subpop/requirements.yaml b/pkg/chartutil/testdata/subpop/requirements.yaml index 5b6b8304e..9840e047d 100644 --- a/pkg/chartutil/testdata/subpop/requirements.yaml +++ b/pkg/chartutil/testdata/subpop/requirements.yaml @@ -12,4 +12,4 @@ dependencies: condition: subchart2.enabled tags: - back-end - - subchart2 \ No newline at end of file + - subchart2 diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go index 074c7a97c..cc60860cd 100644 --- a/pkg/chartutil/values.go +++ b/pkg/chartutil/values.go @@ -383,7 +383,7 @@ func istable(v interface{}) bool { // PathValue takes a yaml path with . notation and returns the value if exists func (v Values) PathValue(ypath string) (interface{}, error) { if len(ypath) == 0 { - return nil, error(fmt.Errorf("yaml path string cannot be zero length")) + return nil, fmt.Errorf("yaml path string cannot be zero length") } yps := strings.Split(ypath, ".") if len(yps) == 1 { @@ -397,16 +397,20 @@ func (v Values) PathValue(ypath string) (interface{}, error) { // key not found return nil, ErrNoValue(fmt.Errorf("%v is not a value", k)) } - table := yps[:len(yps)-1] + // join all elements of YAML path except last to get string table path + ypsLen := len(yps) + table := yps[:ypsLen-1] st := strings.Join(table, ".") - key := yps[len(yps)-1:] + // get the last element as a string key + key := yps[ypsLen-1:] sk := string(key[0]) - + // get our table for table path t, err := v.Table(st) if err != nil { //no table return nil, ErrNoValue(fmt.Errorf("%v is not a value", sk)) } + // check table for key and ensure value is not a table if k, ok := t[sk]; ok && !istable(k) { // key found return k, nil