From 64cc4084d2d05f3251e7178cad039a72c8873c63 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Fri, 26 May 2017 22:47:42 -0700 Subject: [PATCH] helper/schema: ResourceDiff tests, bug fixes The beginning of tests for SetNew. Noticed that removed values are not logged for computed keys in a diff - not too sure if that is going to be a problem (possibly not as GetChange in ResourceData should still reference state for the old value). I came on this most notably in the "TestSetNew/complex-ish_set_diff" test, for reference. More tests pending for other functions for coverage of other functions and test cases as defined in #8769. --- helper/schema/resource_diff.go | 11 +- helper/schema/resource_diff_test.go | 420 ++++++++++++++++++++++++++++ 2 files changed, 426 insertions(+), 5 deletions(-) create mode 100644 helper/schema/resource_diff_test.go diff --git a/helper/schema/resource_diff.go b/helper/schema/resource_diff.go index 7b44f5bbb..de095514e 100644 --- a/helper/schema/resource_diff.go +++ b/helper/schema/resource_diff.go @@ -129,6 +129,7 @@ func newResourceDiff(schema map[string]*Schema, config *terraform.ResourceConfig } // Duplicate the passed in schema to ensure that any changes we make with // functions like ForceNew don't affect the referenced schema. + d.schema = make(map[string]*Schema) for k, v := range schema { newSchema := *v d.schema[k] = &newSchema @@ -192,7 +193,7 @@ func newResourceDiff(schema map[string]*Schema, config *terraform.ResourceConfig } // UpdatedKeys returns the keys that were updated by SetNew, SetNewComputed, or -// SetDiff. These are the only keys that ad iff should be re-calculated for. +// SetDiff. These are the only keys that a diff should be re-calculated for. func (d *ResourceDiff) UpdatedKeys() []string { s := make([]string, 0) for k := range d.updatedKeys { @@ -253,7 +254,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b // doing so will taint any existing diff for this key and will remove it from // the catalog. func (d *ResourceDiff) SetNew(key string, value interface{}) error { - return d.SetDiff(key, d.Get(key), value, false) + return d.SetDiff(key, d.getExact(strings.Split(key, "."), "state").Value, value, false) } // SetNewComputed functions like SetNew, except that it sets the new value to @@ -386,9 +387,9 @@ func (d *ResourceDiff) getChange(key string) (getResult, getResult) { old := d.getExact(strings.Split(key, "."), "newDiffOld") new := d.getExact(strings.Split(key, "."), "newDiffNew") - if old.Exists && new.Exists { - // Both values should exist if SetDiff operated on this key. - // TODO: Maybe verify this. Zero values might be an issue here. + if old.Exists || new.Exists { + // If one of these values exists, then SetDiff has operated on this value, + // and as such the newDiff level should be used. return old, new } diff --git a/helper/schema/resource_diff_test.go b/helper/schema/resource_diff_test.go new file mode 100644 index 000000000..03b93a04d --- /dev/null +++ b/helper/schema/resource_diff_test.go @@ -0,0 +1,420 @@ +package schema + +import ( + "fmt" + "reflect" + "testing" + + "github.com/davecgh/go-spew/spew" + "github.com/hashicorp/terraform/terraform" +) + +// testSetFunc is a very simple function we use to test a foo/bar complex set. +// Both "foo" and "bar" are int values. +// +// This is not foolproof as since it performs sums, you can run into +// collisions. Spec tests accordingly. :P +func testSetFunc(v interface{}) int { + m := v.(map[string]interface{}) + return m["foo"].(int) + m["bar"].(int) +} + +func TestSetNew(t *testing.T) { + testCases := []struct { + Name string + Schema map[string]*Schema + State *terraform.InstanceState + Config *terraform.ResourceConfig + Diff *terraform.InstanceDiff + Key string + NewValue interface{} + Expected *terraform.InstanceDiff + ExpectedError bool + }{ + { + Name: "basic primitive diff", + Schema: map[string]*Schema{ + "foo": &Schema{ + Type: TypeString, + Optional: true, + Computed: true, + }, + }, + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo": "bar", + }, + }, + Config: testConfig(t, map[string]interface{}{ + "foo": "baz", + }), + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "baz", + }, + }, + }, + Key: "foo", + NewValue: "qux", + Expected: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "qux", + }, + }, + }, + }, + { + Name: "basic set diff", + Schema: map[string]*Schema{ + "foo": &Schema{ + Type: TypeSet, + Optional: true, + Computed: true, + Elem: &Schema{Type: TypeString}, + Set: HashString, + }, + }, + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo.#": "1", + "foo.1996459178": "bar", + }, + }, + Config: testConfig(t, map[string]interface{}{ + "foo": []interface{}{"baz"}, + }), + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.1996459178": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "", + NewRemoved: true, + }, + "foo.2015626392": &terraform.ResourceAttrDiff{ + Old: "", + New: "baz", + }, + }, + }, + Key: "foo", + NewValue: []interface{}{"qux"}, + Expected: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.1996459178": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "", + NewRemoved: true, + }, + "foo.2800005064": &terraform.ResourceAttrDiff{ + Old: "", + New: "qux", + }, + }, + }, + }, + { + Name: "basic list diff", + Schema: map[string]*Schema{ + "foo": &Schema{ + Type: TypeList, + Optional: true, + Computed: true, + Elem: &Schema{Type: TypeString}, + }, + }, + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo.#": "1", + "foo.0": "bar", + }, + }, + Config: testConfig(t, map[string]interface{}{ + "foo": []interface{}{"baz"}, + }), + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.0": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "baz", + }, + }, + }, + Key: "foo", + NewValue: []interface{}{"qux"}, + Expected: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.0": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "qux", + }, + }, + }, + }, + { + Name: "basic map diff", + Schema: map[string]*Schema{ + "foo": &Schema{ + Type: TypeMap, + Optional: true, + Computed: true, + }, + }, + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo.%": "1", + "foo.bar": "baz", + }, + }, + Config: testConfig(t, map[string]interface{}{ + "foo": map[string]interface{}{"bar": "qux"}, + }), + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.bar": &terraform.ResourceAttrDiff{ + Old: "baz", + New: "qux", + }, + }, + }, + Key: "foo", + NewValue: map[string]interface{}{"bar": "quux"}, + Expected: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.bar": &terraform.ResourceAttrDiff{ + Old: "baz", + New: "quux", + }, + }, + }, + }, + { + Name: "additional diff with primitive", + Schema: map[string]*Schema{ + "foo": &Schema{ + Type: TypeString, + Optional: true, + }, + "one": &Schema{ + Type: TypeString, + Optional: true, + Computed: true, + }, + }, + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo": "bar", + "one": "two", + }, + }, + Config: testConfig(t, map[string]interface{}{ + "foo": "baz", + "one": "three", + }), + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "baz", + }, + "one": &terraform.ResourceAttrDiff{ + Old: "two", + New: "three", + }, + }, + }, + Key: "one", + NewValue: "four", + Expected: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "baz", + }, + "one": &terraform.ResourceAttrDiff{ + Old: "two", + New: "four", + }, + }, + }, + }, + { + Name: "additional diff with primitive computed only", + Schema: map[string]*Schema{ + "foo": &Schema{ + Type: TypeString, + Optional: true, + }, + "one": &Schema{ + Type: TypeString, + Computed: true, + }, + }, + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo": "bar", + "one": "two", + }, + }, + Config: testConfig(t, map[string]interface{}{ + "foo": "baz", + }), + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "baz", + }, + }, + }, + Key: "one", + NewValue: "three", + Expected: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo": &terraform.ResourceAttrDiff{ + Old: "bar", + New: "baz", + }, + "one": &terraform.ResourceAttrDiff{ + Old: "two", + New: "three", + }, + }, + }, + }, + { + Name: "complex-ish set diff", + Schema: map[string]*Schema{ + "top": &Schema{ + Type: TypeSet, + Optional: true, + Computed: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "foo": &Schema{ + Type: TypeInt, + Optional: true, + Computed: true, + }, + "bar": &Schema{ + Type: TypeInt, + Optional: true, + Computed: true, + }, + }, + }, + Set: testSetFunc, + }, + }, + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "top.#": "2", + "top.3.foo": "1", + "top.3.bar": "2", + "top.23.foo": "11", + "top.23.bar": "12", + }, + }, + Config: testConfig(t, map[string]interface{}{ + "top": []interface{}{ + map[string]interface{}{ + "foo": 1, + "bar": 3, + }, + map[string]interface{}{ + "foo": 12, + "bar": 12, + }, + }, + }), + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "top.4.foo": &terraform.ResourceAttrDiff{ + Old: "", + New: "1", + }, + "top.4.bar": &terraform.ResourceAttrDiff{ + Old: "", + New: "3", + }, + "top.24.foo": &terraform.ResourceAttrDiff{ + Old: "", + New: "12", + }, + "top.24.bar": &terraform.ResourceAttrDiff{ + Old: "", + New: "12", + }, + }, + }, + Key: "top", + NewValue: NewSet(testSetFunc, []interface{}{ + map[string]interface{}{ + "foo": 1, + "bar": 4, + }, + map[string]interface{}{ + "foo": 13, + "bar": 12, + }, + map[string]interface{}{ + "foo": 21, + "bar": 22, + }, + }), + Expected: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "top.#": &terraform.ResourceAttrDiff{ + Old: "2", + New: "3", + }, + "top.5.foo": &terraform.ResourceAttrDiff{ + Old: "", + New: "1", + }, + "top.5.bar": &terraform.ResourceAttrDiff{ + Old: "", + New: "4", + }, + "top.25.foo": &terraform.ResourceAttrDiff{ + Old: "", + New: "13", + }, + "top.25.bar": &terraform.ResourceAttrDiff{ + Old: "", + New: "12", + }, + "top.43.foo": &terraform.ResourceAttrDiff{ + Old: "", + New: "21", + }, + "top.43.bar": &terraform.ResourceAttrDiff{ + Old: "", + New: "22", + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s", tc.Name), func(t *testing.T) { + m := schemaMap(tc.Schema) + d := newResourceDiff(tc.Schema, nil, tc.State, tc.Diff) + if err := d.SetNew(tc.Key, tc.NewValue); err != nil { + t.Fatalf("bad: %s", err) + } + for _, k := range d.UpdatedKeys() { + if err := m.diff(k, m[k], tc.Diff, d, false); err != nil { + t.Fatalf("bad: %s", err) + } + } + if !reflect.DeepEqual(tc.Expected, tc.Diff) { + t.Fatalf("Expected %s, got %s", spew.Sdump(tc.Expected), spew.Sdump(tc.Diff)) + } + }) + } +}