feat(helm): fixup if/ele,remove extra string casts, add comments

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)

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"}
 

pkg/chartutil/testdata/subpop/Chart.yaml

@@ -1,4 +1,4 @@
 apiVersion: v1
 description: A Helm chart for Kubernetes
 name: parentchart
-version: 0.1.0
+version: 0.1.0

pkg/chartutil/testdata/subpop/charts/subchart1/requirements.yaml

@@ -12,4 +12,4 @@ dependencies:
         condition: subchartb.enabled
         tags:
           - front-end
-          - subchartb
+          - subchartb

pkg/chartutil/testdata/subpop/charts/subchart2/requirements.yaml

@@ -12,4 +12,4 @@ dependencies:
         condition: subchartc.enabled
         tags:
           - back-end
-          - subchartc
+          - subchartc

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
+version: 0.1.0

pkg/chartutil/testdata/subpop/noreqs/values.yaml

@@ -24,4 +24,3 @@ resources:
 tags:
   front-end: true
   back-end: false
-

pkg/chartutil/testdata/subpop/requirements.yaml

@@ -12,4 +12,4 @@ dependencies:
         condition: subchart2.enabled
         tags:
           - back-end
-          - subchart2
+          - subchart2

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