Use ParseInto properly to merge --set values. Modify deployment creation funcs to return err. Remove cruft. Needs test for --set value containing list index.

cmd/helm/installer/install.go

@@ -21,10 +21,9 @@ import (
 
 	"strings"
 
-	"log"
+	"fmt"
 
 	"github.com/ghodss/yaml"
-	"github.com/imdario/mergo"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -79,13 +78,17 @@ func Upgrade(client kubernetes.Interface, opts *Options) error {
 
 // createDeployment creates the Tiller deployment reource
 func createDeployment(client extensionsclient.DeploymentsGetter, opts *Options) error {
-	obj := deployment(opts)
+	obj, err := deployment(opts)
-	_, err := client.Deployments(obj.Namespace).Create(obj)
+	if err != nil {
+		return err
+	}
+	_, err = client.Deployments(obj.Namespace).Create(obj)
 	return err
+
 }
 
 // deployment gets the deployment object that installs Tiller.
-func deployment(opts *Options) *v1beta1.Deployment {
+func deployment(opts *Options) (*v1beta1.Deployment, error) {
 	return generateDeployment(opts)
 }
 
@@ -104,7 +107,10 @@ func service(namespace string) *v1.Service {
 // DeploymentManifest gets the manifest (as a string) that describes the Tiller Deployment
 // resource.
 func DeploymentManifest(opts *Options) (string, error) {
-	obj := deployment(opts)
+	obj, err := deployment(opts)
+	if err != nil {
+		return "", err
+	}
 	buf, err := yaml.Marshal(obj)
 	return string(buf), err
 }
@@ -136,100 +142,7 @@ func parseNodeSelectors(labels string) map[string]string {
 
 	return nodeSelectors
 }
-
+func generateDeployment(opts *Options) (*v1beta1.Deployment, error) {
-// istable is a special-purpose function to see if the present thing matches the definition of a YAML table.
-func istable(v interface{}) bool {
-	_, ok := v.(map[string]interface{})
-	return ok
-}
-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 {
-		if istable(val) {
-			if innerdst, ok := dst[key]; !ok {
-				dst[key] = val
-			} else if istable(innerdst) {
-				coalesceTables(innerdst.(map[string]interface{}), val.(map[string]interface{}))
-			} else {
-				log.Printf("warning: cannot overwrite table with non table for %s (%v)", key, val)
-			}
-			continue
-		} else if dv, ok := dst[key]; ok && istable(dv) {
-			log.Printf("warning: destination for %s is a table. Ignoring non-table value %v", key, val)
-			continue
-		} else if !ok { // <- ok is still in scope from preceding conditional.
-			dst[key] = val
-			continue
-		}
-	}
-	return dst
-}
-
-// pathToMap creates a nested map given a YAML path in dot notation.
-func pathToMap(path string, data map[string]interface{}) map[string]interface{} {
-	if path == "." {
-		return data
-	}
-	ap := strings.Split(path, ".")
-	if len(ap) == 0 {
-		return nil
-	}
-	n := []map[string]interface{}{}
-	// created nested map for each key, adding to slice
-	for _, v := range ap {
-		nm := make(map[string]interface{})
-		nm[v] = make(map[string]interface{})
-		n = append(n, nm)
-	}
-	// find the last key (map) and set our data
-	for i, d := range n {
-		for k := range d {
-			z := i + 1
-			if z == len(n) {
-				n[i][k] = data
-				break
-			}
-			n[i][k] = n[z]
-		}
-	}
-
-	return n[0]
-}
-
-// Merges source and destination map, preferring values from the source map
-func mergeValues(dest map[string]interface{}, src map[string]interface{}) map[string]interface{} {
-	for k, v := range src {
-		// If the key doesn't exist already, then just set the key to that value
-		if _, exists := dest[k]; !exists {
-			dest[k] = v
-			continue
-		}
-		nextMap, ok := v.(map[string]interface{})
-		// If it isn't another map, overwrite the value
-		if !ok {
-			dest[k] = v
-			continue
-		}
-		// If the key doesn't exist already, then just set the key to that value
-		if _, exists := dest[k]; !exists {
-			dest[k] = nextMap
-			continue
-		}
-		// Edge case: If the key exists in the destination, but isn't a map
-		destMap, isMap := dest[k].(map[string]interface{})
-		// If the source map has a map for this key, prefer it
-		if !isMap {
-			dest[k] = v
-			continue
-		}
-		// If we got to this point, it is a map in both, so merge them
-		dest[k] = mergeValues(destMap, nextMap)
-	}
-	return dest
-}
-
-func generateDeployment(opts *Options) *v1beta1.Deployment {
 	labels := generateLabels(map[string]string{"name": "tiller"})
 	nodeSelectors := map[string]string{}
 	if len(opts.NodeSelectors) > 0 {
@@ -326,51 +239,32 @@ func generateDeployment(opts *Options) *v1beta1.Deployment {
 		// get YAML from original deployment
 		dy, err := yaml.Marshal(d)
 		if err != nil {
-			log.Fatalf("Error marshalling base Tiller Deployment to YAML: %+v", err)
+			return nil, fmt.Errorf("Error marshalling base Tiller Deployment: %s", err)
 		}
 		// convert deployment YAML to values
 		dv, err := chartutil.ReadValues(dy)
 		if err != nil {
-			log.Fatalf("Error converting Deployment manifest to Values: %+v ", err)
+			return nil, fmt.Errorf("Error converting Deployment manifest: %s ", err)
 		}
-		setMap, err := opts.valuesMap()
-		// transform our set map back into YAML
-		setS, err := yaml.Marshal(setMap)
-
-		if err != nil {
-			log.Fatalf("Error marshalling set map to YAML: %+v ", err)
-		}
-		// transform our YAML into Values
-		setV, err := chartutil.ReadValues(setS)
-
-		//log.Fatal(setV)
-		if err != nil {
-			log.Fatalf("Error reading Values from input: %+v ", err)
-		}
-		// merge original deployment map and set map
-		//finalM := coalesceTables(dv.AsMap(), setV.AsMap())
-		//finalM := mergeValues(dv.AsMap(), setV.AsMap())
 		dm := dv.AsMap()
-		sm := setV.AsMap()
+		// merge --set values into our map
-		err = mergo.Merge(&sm, dm)
+		sm, err := opts.valuesMap(dm)
 		if err != nil {
-			log.Fatal(err)
+			return nil, fmt.Errorf("Error merging --set values into Deployment manifest")
 		}
-		log.Fatal(sm) //for other merges use finalM above
+		finalY, err := yaml.Marshal(sm)
-		finalY, err := yaml.Marshal(dm)
 		if err != nil {
-			log.Fatalf("Error marshalling merged map to YAML: %+v ", err)
+			return nil, fmt.Errorf("Error marshalling merged map to YAML: %s ", err)
 		}
-
 		// convert merged values back into deployment
 		err = yaml.Unmarshal([]byte(finalY), &dd)
 		if err != nil {
-			log.Fatalf("Error unmarshalling Values to Deployment manifest: %+v ", err)
+			return nil, fmt.Errorf("Error unmarshalling Values to Deployment manifest: %s ", err)
 		}
 		d = &dd
 	}
 
-	return d
+	return d, nil
 }
 
 func generateService(namespace string) *v1.Service {

cmd/helm/installer/install_test.go

@@ -331,7 +331,7 @@ func TestInstall_canary(t *testing.T) {
 func TestUpgrade(t *testing.T) {
 	image := "gcr.io/kubernetes-helm/tiller:v2.0.0"
 	serviceAccount := "newServiceAccount"
-	existingDeployment := deployment(&Options{
+	existingDeployment, _ := deployment(&Options{
 		Namespace:      v1.NamespaceDefault,
 		ImageSpec:      "imageToReplace",
 		ServiceAccount: "serviceAccountToReplace",
@@ -372,7 +372,7 @@ func TestUpgrade(t *testing.T) {
 func TestUpgrade_serviceNotFound(t *testing.T) {
 	image := "gcr.io/kubernetes-helm/tiller:v2.0.0"
 
-	existingDeployment := deployment(&Options{
+	existingDeployment, _ := deployment(&Options{
 		Namespace: v1.NamespaceDefault,
 		ImageSpec: "imageToReplace",
 		UseCanary: false,
@@ -471,8 +471,8 @@ func TestDeploymentManifest_WithSetValues(t *testing.T) {
 		expect     interface{}
 	}{
 		{
-			Options{Namespace: v1.NamespaceDefault, Values: []string{"spec.template.spec.nodeselector=app=tiller"}},
+			Options{Namespace: v1.NamespaceDefault, Values: []string{"spec.template.spec.nodeselector.app=tiller"}},
-			"setValues spec.template.spec.nodeSelector=app=tiller",
+			"setValues spec.template.spec.nodeSelector.app=tiller",
 			"spec.template.spec.nodeSelector.app",
 			"tiller",
 		},
@@ -483,11 +483,20 @@ func TestDeploymentManifest_WithSetValues(t *testing.T) {
 			2,
 		},
 		{
-			Options{Namespace: v1.NamespaceDefault, Values: []string{"spec.template.spec=activedeadlineseconds=120"}},
+			Options{Namespace: v1.NamespaceDefault, Values: []string{"spec.template.spec.activedeadlineseconds=120"}},
-			"setValues spec.template.spec=activedeadlineseconds=120",
+			"setValues spec.template.spec.activedeadlineseconds=120",
 			"spec.template.spec.activeDeadlineSeconds",
 			120,
 		},
+		/*
+			// TODO test --set value nested beneath list
+			{
+				Options{Namespace: v1.NamespaceDefault, Values: []string{"spec.template.spec.containers[0].image=gcr.io/kubernetes-helm/tiller:v2.4.2"}},
+				"setValues spec.template.spec.containers[0].image=gcr.io/kubernetes-helm/tiller:v2.4.2",
+				"spec.template.spec.containers[0].image",
+				"gcr.io/kubernetes-helm/tiller:v2.4.2",
+			},
+		*/
 	}
 	for _, tt := range tests {
 		o, err := DeploymentManifest(&tt.opts)

cmd/helm/installer/options.go

@@ -104,8 +104,7 @@ func (opts *Options) pullPolicy() v1.PullPolicy {
 func (opts *Options) tls() bool { return opts.EnableTLS || opts.VerifyTLS }
 
 // valuesMap returns user set values in map format
-func (opts *Options) valuesMap() (map[string]interface{}, error) {
+func (opts *Options) valuesMap(m map[string]interface{}) (map[string]interface{}, error) {
-	m := map[string]interface{}{}
 	for _, skv := range opts.Values {
 		err := strvals.ParseInto(skv, m)
 		if err != nil {