From 8cd0ee80e519ccbb2f6056df2a375cb5c1dc207e Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 31 Aug 2017 11:47:10 -0700 Subject: [PATCH] config: merge/append for local values It seems that this somehow got lost in the commit/rebase shuffle and wasn't caught by the tests that _did_ make it because they were all using just one file. As a result of this bug, locals would fail to work correctly in any configuration with more than one .tf file. Along with restoring the append/merge behavior, this also reworks some of the tests to exercise the multi-file case as better insurance against regressions of this sort in future. This fixes #15969. --- config/append.go | 6 +++ config/append_test.go | 29 ++++++++++---- config/config_test.go | 16 ++++++++ config/merge.go | 15 +++++++ config/merge_test.go | 39 +++++++++++++++---- .../validate-local-multi-file/local-def.tf | 4 ++ .../validate-local-multi-file/local-use.tf | 8 ++++ .../test-fixtures/apply-local-val/main.tf | 8 ---- .../test-fixtures/apply-local-val/outputs.tf | 9 +++++ 9 files changed, 112 insertions(+), 22 deletions(-) create mode 100644 config/test-fixtures/validate-local-multi-file/local-def.tf create mode 100644 config/test-fixtures/validate-local-multi-file/local-use.tf create mode 100644 terraform/test-fixtures/apply-local-val/outputs.tf diff --git a/config/append.go b/config/append.go index 5f4e89eef..9d80c42b7 100644 --- a/config/append.go +++ b/config/append.go @@ -82,5 +82,11 @@ func Append(c1, c2 *Config) (*Config, error) { c.Variables = append(c.Variables, c2.Variables...) } + if len(c1.Locals) > 0 || len(c2.Locals) > 0 { + c.Locals = make([]*Local, 0, len(c1.Locals)+len(c2.Locals)) + c.Locals = append(c.Locals, c1.Locals...) + c.Locals = append(c.Locals, c2.Locals...) + } + return c, nil } diff --git a/config/append_test.go b/config/append_test.go index 17cca25b7..853a460b0 100644 --- a/config/append_test.go +++ b/config/append_test.go @@ -1,8 +1,11 @@ package config import ( + "fmt" "reflect" "testing" + + "github.com/davecgh/go-spew/spew" ) func TestAppend(t *testing.T) { @@ -30,6 +33,9 @@ func TestAppend(t *testing.T) { Variables: []*Variable{ &Variable{Name: "foo"}, }, + Locals: []*Local{ + &Local{Name: "foo"}, + }, unknownKeys: []string{"foo"}, }, @@ -53,6 +59,9 @@ func TestAppend(t *testing.T) { Variables: []*Variable{ &Variable{Name: "bar"}, }, + Locals: []*Local{ + &Local{Name: "bar"}, + }, unknownKeys: []string{"bar"}, }, @@ -81,6 +90,10 @@ func TestAppend(t *testing.T) { &Variable{Name: "foo"}, &Variable{Name: "bar"}, }, + Locals: []*Local{ + &Local{Name: "foo"}, + &Local{Name: "bar"}, + }, unknownKeys: []string{"foo", "bar"}, }, @@ -146,13 +159,15 @@ func TestAppend(t *testing.T) { } for i, tc := range cases { - actual, err := Append(tc.c1, tc.c2) - if err != nil != tc.err { - t.Fatalf("%d: error fail", i) - } + t.Run(fmt.Sprintf("%02d", i), func(t *testing.T) { + actual, err := Append(tc.c1, tc.c2) + if err != nil != tc.err { + t.Errorf("unexpected error: %s", err) + } - if !reflect.DeepEqual(actual, tc.result) { - t.Fatalf("%d: bad:\n\n%#v", i, actual) - } + if !reflect.DeepEqual(actual, tc.result) { + t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(actual), spew.Sdump(tc.result)) + } + }) } } diff --git a/config/config_test.go b/config/config_test.go index edcf36af2..49c4d9ac3 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -653,6 +653,22 @@ func TestNameRegexp(t *testing.T) { } } +func TestConfigValidate_localValuesMultiFile(t *testing.T) { + c, err := LoadDir(filepath.Join(fixtureDir, "validate-local-multi-file")) + if err != nil { + t.Fatalf("unexpected error during load: %s", err) + } + if err := c.Validate(); err != nil { + t.Fatalf("unexpected error from validate: %s", err) + } + if len(c.Locals) != 1 { + t.Fatalf("got 0 locals; want 1") + } + if got, want := c.Locals[0].Name, "test"; got != want { + t.Errorf("wrong local name\ngot: %#v\nwant: %#v", got, want) + } +} + func TestProviderConfigName(t *testing.T) { pcs := []*ProviderConfig{ &ProviderConfig{Name: "aw"}, diff --git a/config/merge.go b/config/merge.go index db214be45..23798d652 100644 --- a/config/merge.go +++ b/config/merge.go @@ -137,6 +137,21 @@ func Merge(c1, c2 *Config) (*Config, error) { } } + // Local Values + // These are simpler than the other config elements because they are just + // flat values and so no deep merging is required. + if localsCount := len(c1.Locals) + len(c2.Locals); localsCount != 0 { + // Explicit length check above because we want c.Locals to remain + // nil if the result would be empty. + c.Locals = make([]*Local, 0, len(c1.Locals)+len(c2.Locals)) + for _, v := range c1.Locals { + c.Locals = append(c.Locals, v) + } + for _, v := range c2.Locals { + c.Locals = append(c.Locals, v) + } + } + return c, nil } diff --git a/config/merge_test.go b/config/merge_test.go index 5cd87aca6..ae7c7eec0 100644 --- a/config/merge_test.go +++ b/config/merge_test.go @@ -1,8 +1,11 @@ package config import ( + "fmt" "reflect" "testing" + + "github.com/davecgh/go-spew/spew" ) func TestMerge(t *testing.T) { @@ -31,6 +34,9 @@ func TestMerge(t *testing.T) { Variables: []*Variable{ &Variable{Name: "foo"}, }, + Locals: []*Local{ + &Local{Name: "foo"}, + }, unknownKeys: []string{"foo"}, }, @@ -54,6 +60,9 @@ func TestMerge(t *testing.T) { Variables: []*Variable{ &Variable{Name: "bar"}, }, + Locals: []*Local{ + &Local{Name: "bar"}, + }, unknownKeys: []string{"bar"}, }, @@ -82,6 +91,10 @@ func TestMerge(t *testing.T) { &Variable{Name: "foo"}, &Variable{Name: "bar"}, }, + Locals: []*Local{ + &Local{Name: "foo"}, + &Local{Name: "bar"}, + }, unknownKeys: []string{"foo", "bar"}, }, @@ -107,6 +120,9 @@ func TestMerge(t *testing.T) { &Variable{Name: "foo", Default: "foo"}, &Variable{Name: "foo"}, }, + Locals: []*Local{ + &Local{Name: "foo"}, + }, unknownKeys: []string{"foo"}, }, @@ -125,6 +141,9 @@ func TestMerge(t *testing.T) { &Variable{Name: "foo", Default: "bar"}, &Variable{Name: "bar"}, }, + Locals: []*Local{ + &Local{Name: "foo"}, + }, unknownKeys: []string{"bar"}, }, @@ -147,6 +166,10 @@ func TestMerge(t *testing.T) { &Variable{Name: "foo"}, &Variable{Name: "bar"}, }, + Locals: []*Local{ + &Local{Name: "foo"}, + &Local{Name: "foo"}, + }, unknownKeys: []string{"foo", "bar"}, }, @@ -462,13 +485,15 @@ func TestMerge(t *testing.T) { } for i, tc := range cases { - actual, err := Merge(tc.c1, tc.c2) - if err != nil != tc.err { - t.Fatalf("%d: error fail", i) - } + t.Run(fmt.Sprintf("%02d", i), func(t *testing.T) { + actual, err := Merge(tc.c1, tc.c2) + if err != nil != tc.err { + t.Errorf("unexpected error: %s", err) + } - if !reflect.DeepEqual(actual, tc.result) { - t.Fatalf("%d: bad:\n\n%#v", i, actual) - } + if !reflect.DeepEqual(actual, tc.result) { + t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(actual), spew.Sdump(tc.result)) + } + }) } } diff --git a/config/test-fixtures/validate-local-multi-file/local-def.tf b/config/test-fixtures/validate-local-multi-file/local-def.tf new file mode 100644 index 000000000..4be4c2329 --- /dev/null +++ b/config/test-fixtures/validate-local-multi-file/local-def.tf @@ -0,0 +1,4 @@ + +locals { + test = "${upper("hello, world")}" +} diff --git a/config/test-fixtures/validate-local-multi-file/local-use.tf b/config/test-fixtures/validate-local-multi-file/local-use.tf new file mode 100644 index 000000000..6ed26c01c --- /dev/null +++ b/config/test-fixtures/validate-local-multi-file/local-use.tf @@ -0,0 +1,8 @@ + +resource "test_resource" "foo" { + value = "${local.test}" +} + +output "test" { + value = "${local.test}" +} diff --git a/terraform/test-fixtures/apply-local-val/main.tf b/terraform/test-fixtures/apply-local-val/main.tf index 67e6053fe..51ca2dedc 100644 --- a/terraform/test-fixtures/apply-local-val/main.tf +++ b/terraform/test-fixtures/apply-local-val/main.tf @@ -8,11 +8,3 @@ locals { result_2 = "${local.result_1}" result_3 = "${local.result_2} world" } - -output "result_1" { - value = "${local.result_1}" -} - -output "result_3" { - value = "${local.result_3}" -} diff --git a/terraform/test-fixtures/apply-local-val/outputs.tf b/terraform/test-fixtures/apply-local-val/outputs.tf new file mode 100644 index 000000000..f0078c190 --- /dev/null +++ b/terraform/test-fixtures/apply-local-val/outputs.tf @@ -0,0 +1,9 @@ +# These are in a separate file to make sure config merging is working properly + +output "result_1" { + value = "${local.result_1}" +} + +output "result_3" { + value = "${local.result_3}" +}