From 861a2ebf26f8aeb6d54c578bf73c06c4ddcdef35 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 16 Apr 2019 15:48:37 -0700 Subject: [PATCH] helper/schema: Use a more targeted shim for nested set diff applying We previously attempted to make the special diff apply behavior for nested sets of objects work with attribute mode by totally discarding attribute mode for all shims. In practice, that is too broad a solution: there are lots of other shimming behaviors that we _don't_ want when attribute mode is enabled. In particular, we need to make sure that the difference between null and empty can be seen in configuration. As a compromise then, we will give all of the shims access to the real ConfigMode and then do a more specialized fixup within the diff-apply logic: we'll construct a synthetic nested block schema and then use that to run our existing logic to deal with nested sets of objects, while using the previous behavior in all other cases. In effect, this means that the special new behavior only applies when the provider uses the opt-in ConfigMode setting on a particular attribute, and thus this change has much less risk of causing broad, unintended regressions elsewhere. --- builtin/providers/test/diff_apply_test.go | 3 +- .../providers/test/resource_config_mode.go | 1 + .../test/resource_config_mode_test.go | 14 ++++++ helper/schema/shims.go | 8 ++-- terraform/diff.go | 44 ++++++++++++++++++- 5 files changed, 64 insertions(+), 6 deletions(-) diff --git a/builtin/providers/test/diff_apply_test.go b/builtin/providers/test/diff_apply_test.go index 4318f0025..63a186d36 100644 --- a/builtin/providers/test/diff_apply_test.go +++ b/builtin/providers/test/diff_apply_test.go @@ -4,6 +4,7 @@ import ( "reflect" "testing" + "github.com/davecgh/go-spew/spew" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" ) @@ -136,6 +137,6 @@ func TestDiffApply_set(t *testing.T) { } if !reflect.DeepEqual(attrs, expected) { - t.Fatalf("\nexpected: %#v\ngot: %#v\n", expected, attrs) + t.Fatalf("wrong result\ngot: %s\nwant: %s\n", spew.Sdump(attrs), spew.Sdump(expected)) } } diff --git a/builtin/providers/test/resource_config_mode.go b/builtin/providers/test/resource_config_mode.go index 48d209447..82e8ff214 100644 --- a/builtin/providers/test/resource_config_mode.go +++ b/builtin/providers/test/resource_config_mode.go @@ -18,6 +18,7 @@ func testResourceConfigMode() *schema.Resource { Type: schema.TypeList, ConfigMode: schema.SchemaConfigModeAttr, Optional: true, + Computed: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "foo": { diff --git a/builtin/providers/test/resource_config_mode_test.go b/builtin/providers/test/resource_config_mode_test.go index 743e8e9e9..02d0c575a 100644 --- a/builtin/providers/test/resource_config_mode_test.go +++ b/builtin/providers/test/resource_config_mode_test.go @@ -99,6 +99,20 @@ resource "test_resource_config_mode" "foo" { }, resource.TestStep{ Config: strings.TrimSpace(` +resource "test_resource_config_mode" "foo" { + resource_as_attr_dynamic = [ + { + }, + ] +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#", "1"), + resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#", "1"), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` resource "test_resource_config_mode" "foo" { resource_as_attr = [] resource_as_attr_dynamic = [] diff --git a/helper/schema/shims.go b/helper/schema/shims.go index e66fc09d4..dc840204b 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -133,9 +133,10 @@ func LegacyResourceSchema(r *Resource) *Resource { return newResource } -// LegacySchema takes a *Schema and returns a deep copy with 0.12 specific -// features removed. This is used by the shims to get a configschema that -// directly matches the structure of the schema.Resource. +// LegacySchema takes a *Schema and returns a deep copy with some 0.12-specific +// features disabled. This is used by the shims to get a configschema that +// better reflects the given schema.Resource, without any adjustments we +// make for when sending a schema to Terraform Core. func LegacySchema(s *Schema) *Schema { if s == nil { return nil @@ -143,7 +144,6 @@ func LegacySchema(s *Schema) *Schema { // start with a shallow copy newSchema := new(Schema) *newSchema = *s - newSchema.ConfigMode = SchemaConfigModeAuto newSchema.SkipCoreTypeCheck = false switch e := newSchema.Elem.(type) { diff --git a/terraform/diff.go b/terraform/diff.go index 00b0052e9..7a6ef3d32 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -646,8 +646,10 @@ func (d *InstanceDiff) applyBlockDiff(path []string, attrs map[string]string, sc func (d *InstanceDiff) applyAttrDiff(path []string, attrs map[string]string, attrSchema *configschema.Attribute) (map[string]string, error) { ty := attrSchema.Type switch { - case ty.IsListType(), ty.IsTupleType(), ty.IsMapType(), ty.IsSetType(): + case ty.IsListType(), ty.IsTupleType(), ty.IsMapType(): return d.applyCollectionDiff(path, attrs, attrSchema) + case ty.IsSetType(): + return d.applySetDiff(path, attrs, attrSchema) default: return d.applySingleAttrDiff(path, attrs, attrSchema) } @@ -873,6 +875,46 @@ func (d *InstanceDiff) applyCollectionDiff(path []string, attrs map[string]strin return result, nil } +func (d *InstanceDiff) applySetDiff(path []string, attrs map[string]string, attrSchema *configschema.Attribute) (map[string]string, error) { + // We only need this special behavior for sets of object. + if !attrSchema.Type.ElementType().IsObjectType() { + // The normal collection apply behavior will work okay for this one, then. + return d.applyCollectionDiff(path, attrs, attrSchema) + } + + // When we're dealing with a set of an object type we actually want to + // use our normal _block type_ apply behaviors, so we'll construct ourselves + // a synthetic schema that treats the object type as a block type and + // then delegate to our block apply method. + synthSchema := &configschema.Block{ + Attributes: make(map[string]*configschema.Attribute), + } + + for name, ty := range attrSchema.Type.ElementType().AttributeTypes() { + // We can safely make everything into an attribute here because in the + // event that there are nested set attributes we'll end up back in + // here again recursively and can then deal with the next level of + // expansion. + synthSchema.Attributes[name] = &configschema.Attribute{ + Type: ty, + Optional: true, + } + } + + parentPath := path[:len(path)-1] + childName := path[len(path)-1] + containerSchema := &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + childName: { + Nesting: configschema.NestingSet, + Block: *synthSchema, + }, + }, + } + + return d.applyBlockDiff(parentPath, attrs, containerSchema) +} + // countFlatmapContainerValues returns the number of values in the flatmapped container // (set, map, list) indexed by key. The key argument is expected to include the // trailing ".#", or ".%".