From 10b3abf405703ce9ce8e2943c29bfd124177931b Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 10 Jun 2015 11:29:44 -0500 Subject: [PATCH 1/4] config: introduce StringList to abstract over list hack This is the initial pure "all tests passing without a diff" stage. The plan is to change the internal representation of StringList to include a suffix delimiter, which will allow us to recognize empty and single-element lists. --- config/interpolate_funcs.go | 31 ++++++++------- config/interpolate_funcs_test.go | 32 ++++++++-------- config/interpolate_walk.go | 10 ++--- config/interpolate_walk_test.go | 4 +- config/string_list.go | 65 ++++++++++++++++++++++++++++++++ helper/schema/schema_test.go | 8 ++-- terraform/interpolate.go | 2 +- 7 files changed, 106 insertions(+), 46 deletions(-) create mode 100644 config/string_list.go diff --git a/config/interpolate_funcs.go b/config/interpolate_funcs.go index 23cc9dfb0210..03bf070b2caf 100644 --- a/config/interpolate_funcs.go +++ b/config/interpolate_funcs.go @@ -131,8 +131,8 @@ func interpolationFuncFormatList() ast.Function { if !ok { continue } - parts := strings.Split(s, InterpSplitDelim) - if len(parts) == 1 { + parts := StringList(s).Slice() + if len(parts) <= 1 { continue } varargs[i-1] = parts @@ -167,7 +167,7 @@ func interpolationFuncFormatList() ast.Function { } list[i] = fmt.Sprintf(format, fmtargs...) } - return strings.Join(list, InterpSplitDelim), nil + return NewStringList(list).String(), nil }, } } @@ -181,7 +181,7 @@ func interpolationFuncJoin() ast.Function { Callback: func(args []interface{}) (interface{}, error) { var list []string for _, arg := range args[1:] { - parts := strings.Split(arg.(string), InterpSplitDelim) + parts := StringList(arg.(string)).Slice() list = append(list, parts...) } @@ -223,18 +223,15 @@ func interpolationFuncLength() ast.Function { ReturnType: ast.TypeInt, Variadic: false, Callback: func(args []interface{}) (interface{}, error) { - if !strings.Contains(args[0].(string), InterpSplitDelim) { + if !IsStringList(args[0].(string)) { return len(args[0].(string)), nil } - var list []string + length := 0 for _, arg := range args { - parts := strings.Split(arg.(string), InterpSplitDelim) - for _, part := range parts { - list = append(list, part) - } + length += StringList(arg.(string)).Length() } - return len(list), nil + return length, nil }, } } @@ -246,7 +243,9 @@ func interpolationFuncSplit() ast.Function { ArgTypes: []ast.Type{ast.TypeString, ast.TypeString}, ReturnType: ast.TypeString, Callback: func(args []interface{}) (interface{}, error) { - return strings.Replace(args[1].(string), args[0].(string), InterpSplitDelim, -1), nil + sep := args[0].(string) + s := args[1].(string) + return NewStringList(strings.Split(s, sep)).String(), nil }, } } @@ -284,7 +283,7 @@ func interpolationFuncElement() ast.Function { ArgTypes: []ast.Type{ast.TypeString, ast.TypeString}, ReturnType: ast.TypeString, Callback: func(args []interface{}) (interface{}, error) { - list := strings.Split(args[0].(string), InterpSplitDelim) + list := StringList(args[0].(string)) index, err := strconv.Atoi(args[1].(string)) if err != nil { @@ -292,7 +291,7 @@ func interpolationFuncElement() ast.Function { "invalid number for index, got %s", args[1]) } - v := list[index%len(list)] + v := list.Element(index) return v, nil }, } @@ -323,7 +322,7 @@ func interpolationFuncKeys(vs map[string]ast.Variable) ast.Function { sort.Strings(keys) - return strings.Join(keys, InterpSplitDelim), nil + return NewStringList(keys).String(), nil }, } } @@ -363,7 +362,7 @@ func interpolationFuncValues(vs map[string]ast.Variable) ast.Function { vals = append(vals, vs[k].Value.(string)) } - return strings.Join(vals, InterpSplitDelim), nil + return NewStringList(vals).String(), nil }, } } diff --git a/config/interpolate_funcs_test.go b/config/interpolate_funcs_test.go index 830a0154cfcd..488aed91cd1e 100644 --- a/config/interpolate_funcs_test.go +++ b/config/interpolate_funcs_test.go @@ -204,7 +204,7 @@ func TestInterpolateFuncFormatList(t *testing.T) { // formatlist applies to each list element in turn { `${formatlist("<%s>", split(",", "A,B"))}`, - "" + InterpSplitDelim + "", + "" + StringListDelim + "", false, }, // formatlist repeats scalar elements @@ -268,8 +268,8 @@ func TestInterpolateFuncJoin(t *testing.T) { fmt.Sprintf(`${join(".", "%s")}`, fmt.Sprintf( "foo%sbar%sbaz", - InterpSplitDelim, - InterpSplitDelim)), + StringListDelim, + StringListDelim)), "foo.bar.baz", false, }, @@ -396,9 +396,9 @@ func TestInterpolateFuncSplit(t *testing.T) { `${split(",", ",,,")}`, fmt.Sprintf( "%s%s%s", - InterpSplitDelim, - InterpSplitDelim, - InterpSplitDelim), + StringListDelim, + StringListDelim, + StringListDelim), false, }, @@ -407,7 +407,7 @@ func TestInterpolateFuncSplit(t *testing.T) { fmt.Sprintf( "%s%s", "foo", - InterpSplitDelim), + StringListDelim), false, }, @@ -415,9 +415,9 @@ func TestInterpolateFuncSplit(t *testing.T) { `${split(",", ",foo,")}`, fmt.Sprintf( "%s%s%s", - InterpSplitDelim, + StringListDelim, "foo", - InterpSplitDelim), + StringListDelim), false, }, @@ -425,8 +425,8 @@ func TestInterpolateFuncSplit(t *testing.T) { `${split(".", "foo.bar.baz")}`, fmt.Sprintf( "foo%sbar%sbaz", - InterpSplitDelim, - InterpSplitDelim), + StringListDelim, + StringListDelim), false, }, }, @@ -486,7 +486,7 @@ func TestInterpolateFuncKeys(t *testing.T) { `${keys("foo")}`, fmt.Sprintf( "bar%squx", - InterpSplitDelim), + StringListDelim), false, }, @@ -535,7 +535,7 @@ func TestInterpolateFuncValues(t *testing.T) { `${values("foo")}`, fmt.Sprintf( "quack%sbaz", - InterpSplitDelim), + StringListDelim), false, }, @@ -568,7 +568,7 @@ func TestInterpolateFuncElement(t *testing.T) { Cases: []testFunctionCase{ { fmt.Sprintf(`${element("%s", "1")}`, - "foo"+InterpSplitDelim+"baz"), + "foo"+StringListDelim+"baz"), "baz", false, }, @@ -582,7 +582,7 @@ func TestInterpolateFuncElement(t *testing.T) { // Invalid index should wrap vs. out-of-bounds { fmt.Sprintf(`${element("%s", "2")}`, - "foo"+InterpSplitDelim+"baz"), + "foo"+StringListDelim+"baz"), "foo", false, }, @@ -590,7 +590,7 @@ func TestInterpolateFuncElement(t *testing.T) { // Too many args { fmt.Sprintf(`${element("%s", "0", "2")}`, - "foo"+InterpSplitDelim+"baz"), + "foo"+StringListDelim+"baz"), nil, true, }, diff --git a/config/interpolate_walk.go b/config/interpolate_walk.go index faacb572621e..a8d8ac7f04a8 100644 --- a/config/interpolate_walk.go +++ b/config/interpolate_walk.go @@ -10,10 +10,6 @@ import ( "github.com/mitchellh/reflectwalk" ) -// InterpSplitDelim is the delimeter that is looked for to split when -// it is returned. -const InterpSplitDelim = `B780FFEC-B661-4EB8-9236-A01737AD98B6` - // interpolationWalker implements interfaces for the reflectwalk package // (github.com/mitchellh/reflectwalk) that can be used to automatically // execute a callback for an interpolation. @@ -149,7 +145,7 @@ func (w *interpolationWalker) Primitive(v reflect.Value) error { // splitting (in a SliceElem) or not. remove := false if w.loc == reflectwalk.SliceElem { - parts := strings.Split(replaceVal, InterpSplitDelim) + parts := StringList(replaceVal).Slice() for _, p := range parts { if p == UnknownVariableValue { remove = true @@ -247,7 +243,7 @@ func (w *interpolationWalker) splitSlice() { if !ok { continue } - if idx := strings.Index(sv, InterpSplitDelim); idx >= 0 { + if IsStringList(sv) { split = true break } @@ -270,7 +266,7 @@ func (w *interpolationWalker) splitSlice() { } // Split on the delimiter - for _, p := range strings.Split(sv, InterpSplitDelim) { + for _, p := range StringList(sv).Slice() { result = append(result, p) } } diff --git a/config/interpolate_walk_test.go b/config/interpolate_walk_test.go index 9b2c34133fc6..5155001e0689 100644 --- a/config/interpolate_walk_test.go +++ b/config/interpolate_walk_test.go @@ -157,7 +157,7 @@ func TestInterpolationWalker_replace(t *testing.T) { "bing", }, }, - Value: "bar" + InterpSplitDelim + "baz", + Value: "bar" + StringListDelim + "baz", }, { @@ -168,7 +168,7 @@ func TestInterpolationWalker_replace(t *testing.T) { }, }, Output: map[string]interface{}{}, - Value: UnknownVariableValue + InterpSplitDelim + "baz", + Value: UnknownVariableValue + StringListDelim + "baz", }, } diff --git a/config/string_list.go b/config/string_list.go new file mode 100644 index 000000000000..1a4ac6a7dbeb --- /dev/null +++ b/config/string_list.go @@ -0,0 +1,65 @@ +package config + +import "strings" + +// StringList represents the "poor man's list" that terraform uses +// internally +type StringList string + +// This is the delimiter used to recognize and split StringLists +const StringListDelim = `B780FFEC-B661-4EB8-9236-A01737AD98B6` + +// Build a StringList from a slice +func NewStringList(parts []string) StringList { + // FOR NOW: + return StringList(strings.Join(parts, StringListDelim)) + // EVENTUALLY: + // var sl StringList + // for _, p := range parts { + // sl = sl.Append(p) + // } + // return sl +} + +// Returns a new StringList with the item appended +func (sl StringList) Append(s string) StringList { + // FOR NOW: + return StringList(strings.Join(append(sl.Slice(), s), StringListDelim)) + // EVENTUALLY: + // return StringList(fmt.Sprintf("%s%s%s", sl, s, StringListDelim)) +} + +// Returns an element at the index, wrapping around the length of the string +// when index > list length +func (sl StringList) Element(index int) string { + return sl.Slice()[index%sl.Length()] +} + +// Returns the length of the StringList +func (sl StringList) Length() int { + return len(sl.Slice()) +} + +// Returns a slice of strings as represented by this StringList +func (sl StringList) Slice() []string { + parts := strings.Split(string(sl), StringListDelim) + + // FOR NOW: + if sl.String() == "" { + return []string{} + } else { + return parts + } + // EVENTUALLY: + // StringLists always have a trailing StringListDelim + // return parts[:len(parts)-1] +} + +func (sl StringList) String() string { + return string(sl) +} + +// Determines if a given string represents a StringList +func IsStringList(s string) bool { + return strings.Contains(s, StringListDelim) +} diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 3fb368ccac9c..2f7c259a50b9 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -582,7 +582,7 @@ func TestSchemaMap_Diff(t *testing.T) { }, ConfigVariables: map[string]string{ - "var.foo": "2" + config.InterpSplitDelim + "5", + "var.foo": "2" + config.StringListDelim + "5", }, Diff: &terraform.InstanceDiff{ @@ -627,7 +627,7 @@ func TestSchemaMap_Diff(t *testing.T) { ConfigVariables: map[string]string{ "var.foo": config.UnknownVariableValue + - config.InterpSplitDelim + "5", + config.StringListDelim + "5", }, Diff: &terraform.InstanceDiff{ @@ -905,7 +905,7 @@ func TestSchemaMap_Diff(t *testing.T) { }, ConfigVariables: map[string]string{ - "var.foo": "2" + config.InterpSplitDelim + "5", + "var.foo": "2" + config.StringListDelim + "5", }, Diff: &terraform.InstanceDiff{ @@ -953,7 +953,7 @@ func TestSchemaMap_Diff(t *testing.T) { ConfigVariables: map[string]string{ "var.foo": config.UnknownVariableValue + - config.InterpSplitDelim + "5", + config.StringListDelim + "5", }, Diff: &terraform.InstanceDiff{ diff --git a/terraform/interpolate.go b/terraform/interpolate.go index a066c0a5f351..83f6265e1eb5 100644 --- a/terraform/interpolate.go +++ b/terraform/interpolate.go @@ -453,7 +453,7 @@ func (i *Interpolater) computeResourceMultiVariable( v.FullKey()) } - return strings.Join(values, config.InterpSplitDelim), nil + return config.NewStringList(values).String(), nil } func (i *Interpolater) resourceVariableInfo( From 7238b3b4afbbe61baa898145972dc1c13691cc84 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 25 Jun 2015 09:48:37 -0500 Subject: [PATCH 2/4] core: encapsulate representation of StringList Now the only code that cares about how StringLists are represented lives inside string_list.go ...which gives us the ability to change it! :) --- config/interpolate_funcs.go | 4 +- config/interpolate_funcs_test.go | 86 +++++++------------------------- config/interpolate_walk_test.go | 4 +- helper/schema/schema_test.go | 12 ++--- 4 files changed, 28 insertions(+), 78 deletions(-) diff --git a/config/interpolate_funcs.go b/config/interpolate_funcs.go index 03bf070b2caf..fbbe441ed2c8 100644 --- a/config/interpolate_funcs.go +++ b/config/interpolate_funcs.go @@ -54,7 +54,7 @@ func interpolationFuncConcat() ast.Function { continue } - if strings.Contains(argument, InterpSplitDelim) { + if IsStringList(argument) { isDeprecated = false } @@ -68,7 +68,7 @@ func interpolationFuncConcat() ast.Function { return b.String(), nil } - return strings.Join(finalList, InterpSplitDelim), nil + return NewStringList(finalList).String(), nil }, } } diff --git a/config/interpolate_funcs_test.go b/config/interpolate_funcs_test.go index 488aed91cd1e..dc6c93339148 100644 --- a/config/interpolate_funcs_test.go +++ b/config/interpolate_funcs_test.go @@ -5,7 +5,6 @@ import ( "io/ioutil" "os" "reflect" - "strings" "testing" "github.com/hashicorp/terraform/config/lang" @@ -42,74 +41,46 @@ func TestInterpolateFuncConcat(t *testing.T) { // String + list { `${concat("a", split(",", "b,c"))}`, - fmt.Sprintf( - "%s%s%s%s%s", - "a", - InterpSplitDelim, - "b", - InterpSplitDelim, - "c"), + NewStringList([]string{"a", "b", "c"}).String(), false, }, // List + string { `${concat(split(",", "a,b"), "c")}`, - fmt.Sprintf( - "%s%s%s%s%s", - "a", - InterpSplitDelim, - "b", - InterpSplitDelim, - "c"), + NewStringList([]string{"a", "b", "c"}).String(), false, }, // Single list { `${concat(split(",", ",foo,"))}`, - fmt.Sprintf( - "%s%s%s", - InterpSplitDelim, - "foo", - InterpSplitDelim), + NewStringList([]string{"", "foo", ""}).String(), false, }, { `${concat(split(",", "a,b,c"))}`, - fmt.Sprintf( - "%s%s%s%s%s", - "a", - InterpSplitDelim, - "b", - InterpSplitDelim, - "c"), + NewStringList([]string{"a", "b", "c"}).String(), false, }, // Two lists { `${concat(split(",", "a,b,c"), split(",", "d,e"))}`, - strings.Join([]string{ - "a", "b", "c", "d", "e", - }, InterpSplitDelim), + NewStringList([]string{"a", "b", "c", "d", "e"}).String(), false, }, // Two lists with different separators { `${concat(split(",", "a,b,c"), split(" ", "d e"))}`, - strings.Join([]string{ - "a", "b", "c", "d", "e", - }, InterpSplitDelim), + NewStringList([]string{"a", "b", "c", "d", "e"}).String(), false, }, // More lists { `${concat(split(",", "a,b"), split(",", "c,d"), split(",", "e,f"), split(",", "0,1"))}`, - strings.Join([]string{ - "a", "b", "c", "d", "e", "f", "0", "1", - }, InterpSplitDelim), + NewStringList([]string{"a", "b", "c", "d", "e", "f", "0", "1"}).String(), false, }, }, @@ -204,7 +175,7 @@ func TestInterpolateFuncFormatList(t *testing.T) { // formatlist applies to each list element in turn { `${formatlist("<%s>", split(",", "A,B"))}`, - "" + StringListDelim + "", + NewStringList([]string{"", ""}).String(), false, }, // formatlist repeats scalar elements @@ -266,10 +237,7 @@ func TestInterpolateFuncJoin(t *testing.T) { { fmt.Sprintf(`${join(".", "%s")}`, - fmt.Sprintf( - "foo%sbar%sbaz", - StringListDelim, - StringListDelim)), + NewStringList([]string{"foo", "bar", "baz"}).String()), "foo.bar.baz", false, }, @@ -394,39 +362,25 @@ func TestInterpolateFuncSplit(t *testing.T) { { `${split(",", ",,,")}`, - fmt.Sprintf( - "%s%s%s", - StringListDelim, - StringListDelim, - StringListDelim), + NewStringList([]string{"", "", "", ""}).String(), false, }, { `${split(",", "foo,")}`, - fmt.Sprintf( - "%s%s", - "foo", - StringListDelim), + NewStringList([]string{"foo", ""}).String(), false, }, { `${split(",", ",foo,")}`, - fmt.Sprintf( - "%s%s%s", - StringListDelim, - "foo", - StringListDelim), + NewStringList([]string{"", "foo", ""}).String(), false, }, { `${split(".", "foo.bar.baz")}`, - fmt.Sprintf( - "foo%sbar%sbaz", - StringListDelim, - StringListDelim), + NewStringList([]string{"foo", "bar", "baz"}).String(), false, }, }, @@ -484,9 +438,7 @@ func TestInterpolateFuncKeys(t *testing.T) { Cases: []testFunctionCase{ { `${keys("foo")}`, - fmt.Sprintf( - "bar%squx", - StringListDelim), + NewStringList([]string{"bar", "qux"}).String(), false, }, @@ -533,9 +485,7 @@ func TestInterpolateFuncValues(t *testing.T) { Cases: []testFunctionCase{ { `${values("foo")}`, - fmt.Sprintf( - "quack%sbaz", - StringListDelim), + NewStringList([]string{"quack", "baz"}).String(), false, }, @@ -568,7 +518,7 @@ func TestInterpolateFuncElement(t *testing.T) { Cases: []testFunctionCase{ { fmt.Sprintf(`${element("%s", "1")}`, - "foo"+StringListDelim+"baz"), + NewStringList([]string{"foo", "baz"}).String()), "baz", false, }, @@ -582,7 +532,7 @@ func TestInterpolateFuncElement(t *testing.T) { // Invalid index should wrap vs. out-of-bounds { fmt.Sprintf(`${element("%s", "2")}`, - "foo"+StringListDelim+"baz"), + NewStringList([]string{"foo", "baz"}).String()), "foo", false, }, @@ -590,7 +540,7 @@ func TestInterpolateFuncElement(t *testing.T) { // Too many args { fmt.Sprintf(`${element("%s", "0", "2")}`, - "foo"+StringListDelim+"baz"), + NewStringList([]string{"foo", "baz"}).String()), nil, true, }, diff --git a/config/interpolate_walk_test.go b/config/interpolate_walk_test.go index 5155001e0689..bc17dbddd2cc 100644 --- a/config/interpolate_walk_test.go +++ b/config/interpolate_walk_test.go @@ -157,7 +157,7 @@ func TestInterpolationWalker_replace(t *testing.T) { "bing", }, }, - Value: "bar" + StringListDelim + "baz", + Value: NewStringList([]string{"bar", "baz"}).String(), }, { @@ -168,7 +168,7 @@ func TestInterpolationWalker_replace(t *testing.T) { }, }, Output: map[string]interface{}{}, - Value: UnknownVariableValue + StringListDelim + "baz", + Value: NewStringList([]string{UnknownVariableValue, "baz"}).String(), }, } diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 2f7c259a50b9..bf5a3a19bdc2 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -582,7 +582,7 @@ func TestSchemaMap_Diff(t *testing.T) { }, ConfigVariables: map[string]string{ - "var.foo": "2" + config.StringListDelim + "5", + "var.foo": config.NewStringList([]string{"2", "5"}).String(), }, Diff: &terraform.InstanceDiff{ @@ -626,8 +626,8 @@ func TestSchemaMap_Diff(t *testing.T) { }, ConfigVariables: map[string]string{ - "var.foo": config.UnknownVariableValue + - config.StringListDelim + "5", + "var.foo": config.NewStringList([]string{ + config.UnknownVariableValue, "5"}).String(), }, Diff: &terraform.InstanceDiff{ @@ -905,7 +905,7 @@ func TestSchemaMap_Diff(t *testing.T) { }, ConfigVariables: map[string]string{ - "var.foo": "2" + config.StringListDelim + "5", + "var.foo": config.NewStringList([]string{"2", "5"}).String(), }, Diff: &terraform.InstanceDiff{ @@ -952,8 +952,8 @@ func TestSchemaMap_Diff(t *testing.T) { }, ConfigVariables: map[string]string{ - "var.foo": config.UnknownVariableValue + - config.StringListDelim + "5", + "var.foo": config.NewStringList([]string{ + config.UnknownVariableValue, "5"}).String(), }, Diff: &terraform.InstanceDiff{ From e88aeede9bf161bb70c32b1308ce49aaea7345a3 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 25 Jun 2015 17:55:51 -0500 Subject: [PATCH 3/4] core: allow distinguishing between empty lists and strings Had to handle a lot of implicit leaning on a few properties of the old representation: * Old representation allowed plain strings to be treated as lists without problem (i.e. shoved into strings.Split), now strings need to be checked whether they are a list before they are treated as one (i.e. shoved into StringList(s).Slice()). * Tested behavior of 0 and 1 length lists in formatlist() was a side effect of the representation. Needs to be special cased now to maintain the behavior. * Found a pretty old context test failure that was wrong in several different ways. It's covered by TestContext2Apply_multiVar so I removed it. --- config/interpolate_funcs.go | 22 ++++++- config/interpolate_funcs_test.go | 14 +++- config/interpolate_walk.go | 13 ++-- config/interpolate_walk_test.go | 2 +- config/string_list.go | 65 +++++++++++-------- terraform/context_test.go | 23 ------- .../plan-var-multi-count-one/main.tf | 9 --- 7 files changed, 78 insertions(+), 70 deletions(-) delete mode 100644 terraform/test-fixtures/plan-var-multi-count-one/main.tf diff --git a/config/interpolate_funcs.go b/config/interpolate_funcs.go index fbbe441ed2c8..22acfb3d4adb 100644 --- a/config/interpolate_funcs.go +++ b/config/interpolate_funcs.go @@ -56,10 +56,11 @@ func interpolationFuncConcat() ast.Function { if IsStringList(argument) { isDeprecated = false + finalList = append(finalList, StringList(argument).Slice()...) + } else { + finalList = append(finalList, argument) } - finalList = append(finalList, argument) - // Deprecated concat behaviour b.WriteString(argument) } @@ -131,11 +132,26 @@ func interpolationFuncFormatList() ast.Function { if !ok { continue } + if !IsStringList(s) { + continue + } + parts := StringList(s).Slice() - if len(parts) <= 1 { + + // 0 or 1 length lists are treated as scalars and repeated + switch len(parts) { + case 0: + varargs[i-1] = "" + continue + case 1: + varargs[i-1] = parts[0] continue } + + // otherwise the list is sent down to be indexed varargs[i-1] = parts + + // Check length if n == 0 { // first list we've seen n = len(parts) diff --git a/config/interpolate_funcs_test.go b/config/interpolate_funcs_test.go index dc6c93339148..e6379fc01425 100644 --- a/config/interpolate_funcs_test.go +++ b/config/interpolate_funcs_test.go @@ -221,7 +221,8 @@ func TestInterpolateFuncJoin(t *testing.T) { }, { - `${join(",", "foo")}`, + fmt.Sprintf(`${join(",", "%s")}`, + NewStringList([]string{"foo"}).String()), "foo", false, }, @@ -354,9 +355,15 @@ func TestInterpolateFuncSplit(t *testing.T) { true, }, + { + `${split(",", "")}`, + NewStringList([]string{""}).String(), + false, + }, + { `${split(",", "foo")}`, - "foo", + NewStringList([]string{"foo"}).String(), false, }, @@ -524,7 +531,8 @@ func TestInterpolateFuncElement(t *testing.T) { }, { - `${element("foo", "0")}`, + fmt.Sprintf(`${element("%s", "0")}`, + NewStringList([]string{"foo"}).String()), "foo", false, }, diff --git a/config/interpolate_walk.go b/config/interpolate_walk.go index a8d8ac7f04a8..adcb5e32cf4d 100644 --- a/config/interpolate_walk.go +++ b/config/interpolate_walk.go @@ -144,7 +144,7 @@ func (w *interpolationWalker) Primitive(v reflect.Value) error { // set if it is computed. This behavior is different if we're // splitting (in a SliceElem) or not. remove := false - if w.loc == reflectwalk.SliceElem { + if w.loc == reflectwalk.SliceElem && IsStringList(replaceVal) { parts := StringList(replaceVal).Slice() for _, p := range parts { if p == UnknownVariableValue { @@ -265,10 +265,15 @@ func (w *interpolationWalker) splitSlice() { continue } - // Split on the delimiter - for _, p := range StringList(sv).Slice() { - result = append(result, p) + if IsStringList(sv) { + for _, p := range StringList(sv).Slice() { + result = append(result, p) + } + continue } + + // Not a string list, so just set it + result = append(result, sv) } // Our slice is now done, we have to replace the slice now diff --git a/config/interpolate_walk_test.go b/config/interpolate_walk_test.go index bc17dbddd2cc..fc7c8b5492f3 100644 --- a/config/interpolate_walk_test.go +++ b/config/interpolate_walk_test.go @@ -183,7 +183,7 @@ func TestInterpolationWalker_replace(t *testing.T) { } if !reflect.DeepEqual(tc.Input, tc.Output) { - t.Fatalf("%d: bad:\n\n%#v", i, tc.Input) + t.Fatalf("%d: bad:\n\nexpected:%#v\ngot:%#v", i, tc.Output, tc.Input) } } } diff --git a/config/string_list.go b/config/string_list.go index 1a4ac6a7dbeb..70d43d1e4bc4 100644 --- a/config/string_list.go +++ b/config/string_list.go @@ -1,37 +1,48 @@ package config -import "strings" +import ( + "fmt" + "strings" +) // StringList represents the "poor man's list" that terraform uses // internally type StringList string // This is the delimiter used to recognize and split StringLists -const StringListDelim = `B780FFEC-B661-4EB8-9236-A01737AD98B6` +// +// It plays two semantic roles: +// * It introduces a list +// * It terminates each element +// +// Example representations: +// [] => SLD +// [""] => SLDSLD +// [" "] => SLD SLD +// ["foo"] => SLDfooSLD +// ["foo", "bar"] => SLDfooSLDbarSLD +// ["", ""] => SLDSLDSLD +const stringListDelim = `B780FFEC-B661-4EB8-9236-A01737AD98B6` // Build a StringList from a slice func NewStringList(parts []string) StringList { - // FOR NOW: - return StringList(strings.Join(parts, StringListDelim)) - // EVENTUALLY: - // var sl StringList - // for _, p := range parts { - // sl = sl.Append(p) - // } - // return sl -} - -// Returns a new StringList with the item appended -func (sl StringList) Append(s string) StringList { - // FOR NOW: - return StringList(strings.Join(append(sl.Slice(), s), StringListDelim)) - // EVENTUALLY: - // return StringList(fmt.Sprintf("%s%s%s", sl, s, StringListDelim)) + // We have to special case the empty list representation + if len(parts) == 0 { + return StringList(stringListDelim) + } + return StringList(fmt.Sprintf("%s%s%s", + stringListDelim, + strings.Join(parts, stringListDelim), + stringListDelim, + )) } // Returns an element at the index, wrapping around the length of the string // when index > list length func (sl StringList) Element(index int) string { + if sl.Length() == 0 { + return "" + } return sl.Slice()[index%sl.Length()] } @@ -42,17 +53,17 @@ func (sl StringList) Length() int { // Returns a slice of strings as represented by this StringList func (sl StringList) Slice() []string { - parts := strings.Split(string(sl), StringListDelim) + parts := strings.Split(string(sl), stringListDelim) - // FOR NOW: - if sl.String() == "" { + switch len(parts) { + case 0, 1: return []string{} - } else { - return parts + case 2: + return []string{""} } - // EVENTUALLY: - // StringLists always have a trailing StringListDelim - // return parts[:len(parts)-1] + + // strip empty elements generated by leading and trailing delimiters + return parts[1 : len(parts)-1] } func (sl StringList) String() string { @@ -61,5 +72,5 @@ func (sl StringList) String() string { // Determines if a given string represents a StringList func IsStringList(s string) bool { - return strings.Contains(s, StringListDelim) + return strings.Contains(s, stringListDelim) } diff --git a/terraform/context_test.go b/terraform/context_test.go index 55580cb9a8e9..53092d72d21c 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -1672,29 +1672,6 @@ func TestContext2Plan_provider(t *testing.T) { } } -func TestContext2Plan_varMultiCountOne(t *testing.T) { - m := testModule(t, "plan-var-multi-count-one") - p := testProvider("aws") - p.DiffFn = testDiffFn - ctx := testContext2(t, &ContextOpts{ - Module: m, - Providers: map[string]ResourceProviderFactory{ - "aws": testProviderFuncFixed(p), - }, - }) - - plan, err := ctx.Plan() - if err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(plan.String()) - expected := strings.TrimSpace(testTerraformPlanVarMultiCountOneStr) - if actual != expected { - t.Fatalf("bad:\n%s", actual) - } -} - func TestContext2Plan_varListErr(t *testing.T) { m := testModule(t, "plan-var-list-err") p := testProvider("aws") diff --git a/terraform/test-fixtures/plan-var-multi-count-one/main.tf b/terraform/test-fixtures/plan-var-multi-count-one/main.tf deleted file mode 100644 index 6fd84932f105..000000000000 --- a/terraform/test-fixtures/plan-var-multi-count-one/main.tf +++ /dev/null @@ -1,9 +0,0 @@ -resource "aws_instance" "foo" { - num = "2" - - count = 1 -} - -resource "aws_instance" "bar" { - foo = "${aws_instance.foo.*.num}" -} From c95f21cec1e7e587b49ea75343a6f36a27e8162a Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 25 Jun 2015 18:45:17 -0500 Subject: [PATCH 4/4] config: make formatlist work on lists of length 1 removes treat-lists-as-scalar special casing for formatlist /cc @radeksimko fixes #2240 --- config/interpolate_funcs.go | 10 ---------- config/interpolate_funcs_test.go | 17 ++++++----------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/config/interpolate_funcs.go b/config/interpolate_funcs.go index 22acfb3d4adb..9b26c24ff2d8 100644 --- a/config/interpolate_funcs.go +++ b/config/interpolate_funcs.go @@ -138,16 +138,6 @@ func interpolationFuncFormatList() ast.Function { parts := StringList(s).Slice() - // 0 or 1 length lists are treated as scalars and repeated - switch len(parts) { - case 0: - varargs[i-1] = "" - continue - case 1: - varargs[i-1] = parts[0] - continue - } - // otherwise the list is sent down to be indexed varargs[i-1] = parts diff --git a/config/interpolate_funcs_test.go b/config/interpolate_funcs_test.go index e6379fc01425..3258bda5c449 100644 --- a/config/interpolate_funcs_test.go +++ b/config/interpolate_funcs_test.go @@ -190,23 +190,18 @@ func TestInterpolateFuncFormatList(t *testing.T) { "A=1, B=2, C=3", false, }, - // formatlist of lists of length zero/one are repeated, just as scalars are - { - `${join(", ", formatlist("%s=%s", split(",", ""), split(",", "1,2,3")))}`, - "=1, =2, =3", - false, - }, - { - `${join(", ", formatlist("%s=%s", split(",", "A"), split(",", "1,2,3")))}`, - "A=1, A=2, A=3", - false, - }, // Mismatched list lengths generate an error { `${formatlist("%s=%2s", split(",", "A,B,C,D"), split(",", "1,2,3"))}`, nil, true, }, + // Works with lists of length 1 [GH-2240] + { + `${formatlist("%s.id", split(",", "demo-rest-elb"))}`, + NewStringList([]string{"demo-rest-elb.id"}).String(), + false, + }, }, }) }