Merge pull request #584 from cloudfoundry/fixing-var-behaviour

Ensure quote stripping behavior for --var matches legacy behaviour.

director/template/var_kv.go

@@ -15,7 +15,6 @@ type VarKV struct {
 func trimWrappingQuotes(s string) string {
 	if (s[0] == '"' && s[len(s)-1] == '"') || (s[0] == '\'' && s[len(s)-1] == '\'') {
 		s = s[1 : len(s)-1]
-		s = trimWrappingQuotes(s)
 	}
 	return s
 }
@@ -37,11 +36,15 @@ func (a *VarKV) UnmarshalFlag(data string) error {
 	var vars interface{}
 
 	err := yaml.Unmarshal([]byte(pieces[valueIndex]), &vars)
-	//If for whatever reason YAML unmarshal fails, treat the second part of pieces as a string
+
+	if err != nil {
+		return bosherr.WrapErrorf(err, "Deserializing variables '%s'", data)
+	}
+
 	//yaml.Unmarshal returns a string if the input is not valid yaml.
-	if _, ok := vars.(string); ok || err != nil {
-		//in that case, we return the initial flag value as the Unmarshal
-		//process strips newlines. Stripping the quotes is required to keep
+	//in that case, we pass through the string itself as the Unmarshal process strips newlines.
+	if _, ok := vars.(string); ok {
+		//Stripping the quotes is required to keep
 		//backwards compability (YAML unmarshal also removed wrapping quotes from the value).
 		*a = VarKV{Name: pieces[nameIndex], Value: trimWrappingQuotes(pieces[valueIndex])}
 	} else {

director/template/var_kv_test.go

@@ -23,32 +23,39 @@ var _ = Describe("VarKV", func() {
 			Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"}))
 		})
 
-		It("reverts to the old (incorrect) bevaviour and removes the wrapping double quotes if the value is a string", func() {
+		It("reverts to the old (incorrect) behaviour and removes the wrapping double quotes if the value is a string", func() {
 			err := (&arg).UnmarshalFlag(`name="val"`)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"}))
 		})
-		It("reverts to the old (incorrect) bevaviour and removes the wrapping single quotes if the value is a string", func() {
+		It("reverts to the old (incorrect) behaviour and removes the wrapping single quotes if the value is a string", func() {
 			err := (&arg).UnmarshalFlag(`name='val'`)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"}))
 		})
-		It("only removes wrapping quotes", func() {
-			err := (&arg).UnmarshalFlag(`name="'val""val'"`)
+		It("fails if the string starts with unmatched quoting", func() {
+			err := (&arg).UnmarshalFlag(`name="my-password'`)
+			Expect(err).To(HaveOccurred())
+		})
+		It("only ever removes a single layer of wrapping quotes", func() {
+			err := (&arg).UnmarshalFlag(`name='""'`)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(arg).To(Equal(VarKV{Name: "name", Value: `val""val`}))
-
-			err = (&arg).UnmarshalFlag(`name="val''val"`)
+			Expect(arg).To(Equal(VarKV{Name: "name", Value: `""`}))
+		})
+		It("only removes wrapping quotes on an empty quoted value", func() {
+			err := (&arg).UnmarshalFlag(`name="''"`)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(arg).To(Equal(VarKV{Name: "name", Value: `val''val`}))
-
-			err = (&arg).UnmarshalFlag(`name='"some-data'"`)
+			Expect(arg).To(Equal(VarKV{Name: "name", Value: `''`}))
+		})
+		It("only removes wrapping quotes on a quoted value", func() {
+			err := (&arg).UnmarshalFlag(`name='"mocked-value"'`)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(arg).To(Equal(VarKV{Name: "name", Value: `'"some-data'"`}))
-
-			err = (&arg).UnmarshalFlag(`name="'some-data"'`)
+			Expect(arg).To(Equal(VarKV{Name: "name", Value: `"mocked-value"`}))
+		})
+		It("removes the quotes on a quoted blank string", func() {
+			err := (&arg).UnmarshalFlag(`name=''`)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(arg).To(Equal(VarKV{Name: "name", Value: `"'some-data"'`}))
+			Expect(arg).To(Equal(VarKV{Name: "name", Value: ``}))
 		})
 		It("sets name and value when value contains a `=`", func() {
 			err := (&arg).UnmarshalFlag("public_key=ssh-rsa G4/+VHa1aw==")