From 8a29c90583177c0e23d3a6410df591edcfb2eb4a Mon Sep 17 00:00:00 2001 From: Justin Scott Date: Fri, 16 Jun 2017 15:14:04 -0700 Subject: [PATCH] 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 | 150 +++++------------------------ cmd/helm/installer/install_test.go | 21 ++-- cmd/helm/installer/options.go | 3 +- 3 files changed, 38 insertions(+), 136 deletions(-) diff --git a/cmd/helm/installer/install.go b/cmd/helm/installer/install.go index c7cd44326..0e747e00d 100644 --- a/cmd/helm/installer/install.go +++ b/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 resource. func createDeployment(client extensionsclient.DeploymentsGetter, opts *Options) error { - obj := deployment(opts) - _, err := client.Deployments(obj.Namespace).Create(obj) + obj, err := deployment(opts) + 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 } - -// 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 { +func generateDeployment(opts *Options) (*v1beta1.Deployment, error) { 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) - } - 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) + return nil, fmt.Errorf("Error converting Deployment manifest: %s ", 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() - err = mergo.Merge(&sm, dm) + // merge --set values into our map + 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(dm) + finalY, err := yaml.Marshal(sm) 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 { diff --git a/cmd/helm/installer/install_test.go b/cmd/helm/installer/install_test.go index dd10795f3..330de04fe 100644 --- a/cmd/helm/installer/install_test.go +++ b/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"}}, - "setValues 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", "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"}}, - "setValues spec.template.spec=activedeadlineseconds=120", + Options{Namespace: v1.NamespaceDefault, Values: []string{"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) diff --git a/cmd/helm/installer/options.go b/cmd/helm/installer/options.go index 47eb456bb..83dcd6813 100644 --- a/cmd/helm/installer/options.go +++ b/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) { - m := map[string]interface{}{} +func (opts *Options) valuesMap(m map[string]interface{}) (map[string]interface{}, error) { for _, skv := range opts.Values { err := strvals.ParseInto(skv, m) if err != nil {