fix(helm): improve --set parser

This replaces the old set parser with a brand new one. This also changes
the internal algorithm from duplicating YAML to merging YAML, which
might solve a problem one user reported in chat, but which was never
captured in an issue.

Closes #1540
Closes #1556

cmd/helm/install.go

@@ -24,7 +24,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
 
 	"text/template"
@@ -35,6 +34,7 @@ import (
 
 	"k8s.io/helm/cmd/helm/downloader"
 	"k8s.io/helm/cmd/helm/helmpath"
+	"k8s.io/helm/cmd/helm/strvals"
 	"k8s.io/helm/pkg/helm"
 	"k8s.io/helm/pkg/kube"
 	"k8s.io/helm/pkg/proto/hapi/release"
@@ -95,7 +95,7 @@ type installCmd struct {
 	keyring      string
 	out          io.Writer
 	client       helm.Interface
-	values       *values
+	values       string
 	nameTemplate string
 	version      string
 }
@@ -104,7 +104,6 @@ func newInstallCmd(c helm.Interface, out io.Writer) *cobra.Command {
 	inst := &installCmd{
 		out:    out,
 		client: c,
-		values: new(values),
 	}
 
 	cmd := &cobra.Command{
@@ -133,7 +132,7 @@ func newInstallCmd(c helm.Interface, out io.Writer) *cobra.Command {
 	f.BoolVar(&inst.dryRun, "dry-run", false, "simulate an install")
 	f.BoolVar(&inst.disableHooks, "no-hooks", false, "prevent hooks from running during install")
 	f.BoolVar(&inst.replace, "replace", false, "re-use the given name, even if that name is already used. This is unsafe in production")
-	f.Var(inst.values, "set", "set values on the command line. Separate values with commas: key1=val1,key2=val2")
+	f.StringVar(&inst.values, "set", "", "set values on the command line. Separate values with commas: key1=val1,key2=val2")
 	f.StringVar(&inst.nameTemplate, "name-template", "", "specify template used to name the release")
 	f.BoolVar(&inst.verify, "verify", false, "verify the package before installing it")
 	f.StringVar(&inst.keyring, "keyring", defaultKeyring(), "location of public keys used for verification")
@@ -199,7 +198,7 @@ func (i *installCmd) run() error {
 }
 
 func (i *installCmd) vals() ([]byte, error) {
-	var buffer bytes.Buffer
+	base := map[string]interface{}{}
 
 	// User specified a values file via -f/--values
 	if i.valuesFile != "" {
@@ -207,24 +206,17 @@ func (i *installCmd) vals() ([]byte, error) {
 		if err != nil {
 			return []byte{}, err
 		}
-		buffer.Write(bytes)
 
-		// Force a new line. An extra won't matter, but a missing one can
+		if err := yaml.Unmarshal(bytes, &base); err != nil {
-		// break things. https://github.com/kubernetes/helm/issues/1430
+			return []byte{}, fmt.Errorf("failed to parse %s: %s", i.valuesFile, err)
-		buffer.WriteRune('\n')
+		}
 	}
 
-	// User specified value pairs via --set
+	if err := strvals.ParseInto(i.values, base); err != nil {
-	// These override any values in the specified file
+		return []byte{}, fmt.Errorf("failed parsing --set data: %s", err)
-	if len(i.values.pairs) > 0 {
-		bytes, err := i.values.yaml()
-		if err != nil {
-			return []byte{}, err
-		}
-		buffer.Write(bytes)
 	}
 
-	return buffer.Bytes(), nil
+	return yaml.Marshal(base)
 }
 
 // printRelease prints info about a release if the flagDebug is true.
@@ -243,81 +235,6 @@ func (i *installCmd) printRelease(rel *release.Release) {
 	}
 }
 
-// values represents the command-line value pairs
-type values struct {
-	pairs map[string]interface{}
-}
-
-func (v *values) yaml() ([]byte, error) {
-	return yaml.Marshal(v.pairs)
-}
-
-func (v *values) String() string {
-	out, _ := v.yaml()
-	return string(out)
-}
-
-func (v *values) Type() string {
-	// Added to pflags.Value interface, but not documented there.
-	return "struct"
-}
-
-func (v *values) Set(data string) error {
-	v.pairs = map[string]interface{}{}
-
-	items := strings.Split(data, ",")
-	for _, item := range items {
-		n, val := splitPair(item)
-		names := strings.Split(n, ".")
-		ln := len(names)
-		current := &v.pairs
-		for i := 0; i < ln; i++ {
-			if i+1 == ln {
-				// We're at the last element. Set it.
-				(*current)[names[i]] = val
-			} else {
-				//
-				if e, ok := (*current)[names[i]]; !ok {
-					m := map[string]interface{}{}
-					(*current)[names[i]] = m
-					current = &m
-				} else if m, ok := e.(map[string]interface{}); ok {
-					current = &m
-				}
-			}
-		}
-	}
-	return nil
-}
-
-func typedVal(val string) interface{} {
-	if strings.EqualFold(val, "true") {
-		return true
-	}
-
-	if strings.EqualFold(val, "false") {
-		return false
-	}
-
-	if iv, err := strconv.ParseInt(val, 10, 64); err == nil {
-		return iv
-	}
-
-	if fv, err := strconv.ParseFloat(val, 64); err == nil {
-		return fv
-	}
-
-	return val
-}
-
-func splitPair(item string) (name string, value interface{}) {
-	pair := strings.SplitN(item, "=", 2)
-	if len(pair) == 1 {
-		return pair[0], true
-	}
-	return pair[0], typedVal(pair[1])
-}
-
 // locateChartPath looks for a chart directory in known places, and returns either the full path or an error.
 //
 // This does not ensure that the chart is well-formed; only that the requested filename exists.

cmd/helm/install_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package main
 
 import (
-	"fmt"
 	"io"
 	"regexp"
 	"strings"
@@ -99,71 +98,6 @@ func TestInstall(t *testing.T) {
 	})
 }
 
-func TestValues(t *testing.T) {
-	args := "sailor=sinbad,good,port.source=baghdad,port.destination=basrah,success=True"
-	vobj := new(values)
-	vobj.Set(args)
-
-	if vobj.Type() != "struct" {
-		t.Fatalf("Expected Type to be struct, got %s", vobj.Type())
-	}
-
-	vals := vobj.pairs
-	if fmt.Sprint(vals["good"]) != "true" {
-		t.Errorf("Expected good to be true. Got %v", vals["good"])
-	}
-
-	if !vals["success"].(bool) {
-		t.Errorf("Expected boolean true. Got %T, %v", vals["success"], vals["success"])
-	}
-
-	port := vals["port"].(map[string]interface{})
-
-	if fmt.Sprint(port["source"]) != "baghdad" {
-		t.Errorf("Expected source to be baghdad. Got %s", port["source"])
-	}
-	if fmt.Sprint(port["destination"]) != "basrah" {
-		t.Errorf("Expected source to be baghdad. Got %s", port["source"])
-	}
-
-	y := `good: true
-port:
-  destination: basrah
-  source: baghdad
-sailor: sinbad
-success: true
-`
-	out, err := vobj.yaml()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if string(out) != y {
-		t.Errorf("Expected YAML to be \n%s\nGot\n%s\n", y, out)
-	}
-
-	if vobj.String() != y {
-		t.Errorf("Expected String() to be \n%s\nGot\n%s\n", y, out)
-	}
-
-	// Combined case, overriding a property
-	vals["sailor"] = "pisti"
-	updatedYAML := `good: true
-port:
-  destination: basrah
-  source: baghdad
-sailor: pisti
-success: true
-`
-	newOut, err := vobj.yaml()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if string(newOut) != updatedYAML {
-		t.Errorf("Expected YAML to be \n%s\nGot\n%s\n", updatedYAML, newOut)
-	}
-
-}
-
 type nameTemplateTestCase struct {
 	tpl              string
 	expected         string

cmd/helm/strvals/doc.go

@@ -0,0 +1,32 @@
+/*
+Copyright 2016 The Kubernetes Authors All rights reserved.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+/*Package strvals provides tools for working with strval lines.
+
+Helm supports a compressed format for YAML settings which we call strvals.
+The format is roughly like this:
+
+	name=value,topname.subname=value
+
+The above is equivalent to the YAML document
+
+	name: value
+	topname:
+	  subname: value
+
+This package provides a parser and utilities for converting the strvals format
+to other formats.
+*/
+package strvals

cmd/helm/strvals/parser.go

@@ -0,0 +1,166 @@
+/*
+Copyright 2016 The Kubernetes Authors All rights reserved.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package strvals
+
+import (
+	"bytes"
+	"strconv"
+	"strings"
+
+	"github.com/ghodss/yaml"
+)
+
+// ToYAML takes a string of arguments and converts to a YAML document.
+func ToYAML(s string) (string, error) {
+	m, err := Parse(s)
+	if err != nil {
+		return "", err
+	}
+	d, err := yaml.Marshal(m)
+	return string(d), err
+}
+
+// Parse parses a set line.
+//
+// A set line is of the form name1=value1,name2=value2
+func Parse(s string) (map[string]interface{}, error) {
+	vals := map[string]interface{}{}
+	scanner := bytes.NewBufferString(s)
+	t := newParser(scanner, vals)
+	t.parse()
+	return vals, nil
+}
+
+//ParseInto parses a strvals line and merges the result into dest.
+//
+// If the strval string has a key that exists in dest, it overwrites the
+// dest version.
+func ParseInto(s string, dest map[string]interface{}) error {
+	scanner := bytes.NewBufferString(s)
+	t := newParser(scanner, dest)
+	t.parse()
+	return nil
+}
+
+// parser is a simple parser that takes a strvals line and parses it into a
+// map representation.
+type parser struct {
+	sc      *bytes.Buffer
+	data    map[string]interface{}
+	carryOn bool
+}
+
+func newParser(sc *bytes.Buffer, data map[string]interface{}) *parser {
+	return &parser{sc: sc, data: data, carryOn: true}
+}
+
+func (t *parser) parse() error {
+	// Starting state is consume key.
+	for t.sc.Len() > 0 {
+		t.key(t.data)
+	}
+	return nil
+}
+
+func (t *parser) key(data map[string]interface{}) {
+	k := []rune{}
+	for {
+		switch r, _, e := t.sc.ReadRune(); {
+		case e != nil:
+			set(data, string(k), "")
+			return
+		case r == '\\':
+			//Escape char. Consume next and append.
+			next, _, e := t.sc.ReadRune()
+			if e != nil {
+				return
+			}
+			k = append(k, next)
+		case r == '=':
+			//End of key. Consume =, Get value.
+			v := t.val()
+			set(data, string(k), typedVal(v))
+			return
+		case r == ',':
+			// No value given. Set the value to empty string.
+			set(data, string(k), "")
+			return
+		case r == '.':
+			// This is the difficult case. Now we're working with a subkey.
+
+			// First, create or find the target map.
+			inner := map[string]interface{}{}
+			if _, ok := data[string(k)]; ok {
+				inner = data[string(k)].(map[string]interface{})
+			}
+
+			// Recurse
+			t.key(inner)
+			set(data, string(k), inner)
+			return
+		default:
+			k = append(k, r)
+		}
+	}
+}
+
+func set(data map[string]interface{}, key string, val interface{}) {
+	// If key is empty, don't set it.
+	if len(key) == 0 {
+		return
+	}
+	data[key] = val
+}
+
+func (t *parser) val() []rune {
+	v := []rune{}
+	for {
+		switch r, _, e := t.sc.ReadRune(); {
+		case e != nil:
+			// End of input or error with reader stops value parsing.
+			return v
+		case r == '\\':
+			//Escape char. Consume next and append.
+			next, _, e := t.sc.ReadRune()
+			if e != nil {
+				return v
+			}
+			v = append(v, next)
+		case r == ',':
+			//End of key. Consume ',' and return.
+			return v
+		default:
+			v = append(v, r)
+		}
+	}
+}
+
+func typedVal(v []rune) interface{} {
+	val := string(v)
+	if strings.EqualFold(val, "true") {
+		return true
+	}
+
+	if strings.EqualFold(val, "false") {
+		return false
+	}
+
+	if iv, err := strconv.ParseInt(val, 10, 64); err == nil {
+		return iv
+	}
+
+	return val
+}

cmd/helm/strvals/parser_test.go

@@ -0,0 +1,168 @@
+/*
+Copyright 2016 The Kubernetes Authors All rights reserved.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package strvals
+
+import (
+	"testing"
+
+	"github.com/ghodss/yaml"
+)
+
+func TestParseSet(t *testing.T) {
+	tests := []struct {
+		str    string
+		expect map[string]interface{}
+	}{
+		{
+			"name1=value1",
+			map[string]interface{}{"name1": "value1"},
+		},
+		{
+			"name1=value1,name2=value2",
+			map[string]interface{}{"name1": "value1", "name2": "value2"},
+		},
+		{
+			"name1=value1,name2=value2,",
+			map[string]interface{}{"name1": "value1", "name2": "value2"},
+		},
+		{
+			"name1=value1,,,,name2=value2,",
+			map[string]interface{}{"name1": "value1", "name2": "value2"},
+		},
+		{
+			"name1=,name2=value2",
+			map[string]interface{}{"name1": "", "name2": "value2"},
+		},
+		{
+			"name1,name2=",
+			map[string]interface{}{"name1": "", "name2": ""},
+		},
+		{
+			"name1,name2=value2",
+			map[string]interface{}{"name1": "", "name2": "value2"},
+		},
+		{
+			"name1,name2=value2\\",
+			map[string]interface{}{"name1": "", "name2": "value2"},
+		},
+		{
+			"name1,name2",
+			map[string]interface{}{"name1": "", "name2": ""},
+		},
+		{
+			"name1=one\\,two,name2=three\\,four",
+			map[string]interface{}{"name1": "one,two", "name2": "three,four"},
+		},
+		{
+			"name1=one\\=two,name2=three\\=four",
+			map[string]interface{}{"name1": "one=two", "name2": "three=four"},
+		},
+		{
+			"name1=one two three,name2=three two one",
+			map[string]interface{}{"name1": "one two three", "name2": "three two one"},
+		},
+		{
+			"outer.inner=value",
+			map[string]interface{}{"outer": map[string]interface{}{"inner": "value"}},
+		},
+		{
+			"outer.middle.inner=value",
+			map[string]interface{}{"outer": map[string]interface{}{"middle": map[string]interface{}{"inner": "value"}}},
+		},
+		{
+			"outer.inner1=value,outer.inner2=value2",
+			map[string]interface{}{"outer": map[string]interface{}{"inner1": "value", "inner2": "value2"}},
+		},
+		{
+			"outer.inner1=value,outer.middle.inner=value",
+			map[string]interface{}{
+				"outer": map[string]interface{}{
+					"inner1": "value",
+					"middle": map[string]interface{}{
+						"inner": "value",
+					},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		got, err := Parse(tt.str)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		y1, err := yaml.Marshal(tt.expect)
+		if err != nil {
+			t.Fatal(err)
+		}
+		y2, err := yaml.Marshal(got)
+		if err != nil {
+			t.Fatalf("Error serializing parsed value: %s", err)
+		}
+
+		if string(y1) != string(y2) {
+			t.Errorf("%s: Expected:\n%s\nGot:\n%s", tt.str, y1, y2)
+		}
+	}
+}
+
+func TestParseInto(t *testing.T) {
+	got := map[string]interface{}{
+		"outer": map[string]interface{}{
+			"inner1": "overwrite",
+			"inner2": "value2",
+		},
+	}
+	input := "outer.inner1=value1,outer.inner3=value3"
+	expect := map[string]interface{}{
+		"outer": map[string]interface{}{
+			"inner1": "value1",
+			"inner2": "value2",
+			"inner3": "value3",
+		},
+	}
+
+	if err := ParseInto(input, got); err != nil {
+		t.Fatal(err)
+	}
+
+	y1, err := yaml.Marshal(expect)
+	if err != nil {
+		t.Fatal(err)
+	}
+	y2, err := yaml.Marshal(got)
+	if err != nil {
+		t.Fatalf("Error serializing parsed value: %s", err)
+	}
+
+	if string(y1) != string(y2) {
+		t.Errorf("%s: Expected:\n%s\nGot:\n%s", input, y1, y2)
+	}
+}
+
+func TestToYAML(t *testing.T) {
+	// The TestParse does the hard part. We just verify that YAML formatting is
+	// happening.
+	o, err := ToYAML("name=value")
+	if err != nil {
+		t.Fatal(err)
+	}
+	expect := "name: value\n"
+	if o != expect {
+		t.Errorf("Expected %q, got %q", expect, o)
+	}
+}

cmd/helm/upgrade.go

@@ -17,14 +17,15 @@ limitations under the License.
 package main
 
 import (
-	"bytes"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"strings"
 
+	"github.com/ghodss/yaml"
 	"github.com/spf13/cobra"
 
+	"k8s.io/helm/cmd/helm/strvals"
 	"k8s.io/helm/pkg/helm"
 	"k8s.io/helm/pkg/storage/driver"
 )
@@ -49,7 +50,7 @@ type upgradeCmd struct {
 	dryRun       bool
 	disableHooks bool
 	valuesFile   string
-	values       *values
+	values       string
 	verify       bool
 	keyring      string
 	install      bool
@@ -62,7 +63,6 @@ func newUpgradeCmd(client helm.Interface, out io.Writer) *cobra.Command {
 	upgrade := &upgradeCmd{
 		out:    out,
 		client: client,
-		values: new(values),
 	}
 
 	cmd := &cobra.Command{
@@ -86,7 +86,7 @@ func newUpgradeCmd(client helm.Interface, out io.Writer) *cobra.Command {
 	f := cmd.Flags()
 	f.StringVarP(&upgrade.valuesFile, "values", "f", "", "path to a values YAML file")
 	f.BoolVar(&upgrade.dryRun, "dry-run", false, "simulate an upgrade")
-	f.Var(upgrade.values, "set", "set values on the command line. Separate values with commas: key1=val1,key2=val2")
+	f.StringVar(&upgrade.values, "set", "", "set values on the command line. Separate values with commas: key1=val1,key2=val2")
 	f.BoolVar(&upgrade.disableHooks, "disable-hooks", false, "disable pre/post upgrade hooks")
 	f.BoolVar(&upgrade.verify, "verify", false, "verify the provenance of the chart before upgrading")
 	f.StringVar(&upgrade.keyring, "keyring", defaultKeyring(), "path to the keyring that contains public singing keys")
@@ -154,7 +154,7 @@ func (u *upgradeCmd) run() error {
 }
 
 func (u *upgradeCmd) vals() ([]byte, error) {
-	var buffer bytes.Buffer
+	base := map[string]interface{}{}
 
 	// User specified a values file via -f/--values
 	if u.valuesFile != "" {
@@ -162,18 +162,15 @@ func (u *upgradeCmd) vals() ([]byte, error) {
 		if err != nil {
 			return []byte{}, err
 		}
-		buffer.Write(bytes)
-	}
 
-	// User specified value pairs via --set
+		if err := yaml.Unmarshal(bytes, base); err != nil {
-	// These override any values in the specified file
+			return []byte{}, fmt.Errorf("failed to parse %s: %s", u.valuesFile, err)
-	if len(u.values.pairs) > 0 {
-		bytes, err := u.values.yaml()
-		if err != nil {
-			return []byte{}, err
 		}
-		buffer.Write(bytes)
 	}
 
-	return buffer.Bytes(), nil
+	if err := strvals.ParseInto(u.values, base); err != nil {
+		return []byte{}, fmt.Errorf("failed parsing --set data: %s", err)
+	}
+
+	return yaml.Marshal(base)
 }