From 270bbdb19bb77f2a74ba268167be855ce2df2b2c Mon Sep 17 00:00:00 2001 From: Jake Champlin Date: Wed, 2 Aug 2017 11:18:59 -0400 Subject: [PATCH 1/4] core: Add `GetOkRaw` schema function Adds `GetOkRaw` as a schema function. This should only be used to verify boolean attributes are either set or not set, regardless of their zero value for their type. There are a few small use cases outside of the boolean type where this will be helpful as well. Overall, this shouldn't detract from the zero-value checks that `GetOK()` currently has, and should only be used when absolutely needed. However, there are enough use-cases for this addition without checking for the zero-value of the type, that this is needed. Primary use case is for a boolean attribute that is `Optional` and `Computed`, without a default value. There's currently no way to verify that the boolean attribute was explicitly set to the zero-value literal with the current `GetOk()` function. This new function allows for that check, keeping the `Computed` check for the returned `exists` boolean. ``` $ make test TEST=./helper/schema TESTARGS="-run=TestResourceDataGetOkRaw" ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2017/08/02 11:17:32 Generated command/internal_plugin_list.go go test -i ./helper/schema || exit 1 echo ./helper/schema | \ xargs -t -n4 go test -run=TestResourceDataGetOkRaw -timeout=60s -parallel=4 go test -run=TestResourceDataGetOkRaw -timeout=60s -parallel=4 ./helper/schema ok github.com/hashicorp/terraform/helper/schema 0.005s ``` --- helper/schema/resource_data.go | 16 +++ helper/schema/resource_data_test.go | 198 ++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+) diff --git a/helper/schema/resource_data.go b/helper/schema/resource_data.go index b2bc8f6c7c..fcb44a2f81 100644 --- a/helper/schema/resource_data.go +++ b/helper/schema/resource_data.go @@ -104,6 +104,22 @@ func (d *ResourceData) GetOk(key string) (interface{}, bool) { return r.Value, exists } +// GetOkRaw returns the data for a given key and whether or not the key +// has been set to a non-zero value. This is only useful for determining +// if boolean attributes have been set, if they are Optional but do not +// have a Default value. +// +// This is nearly the same function as GetOk, yet it does not check +// for the zero value of the attribute's type. This allows for attributes +// without a default, to fully check for a literal assignment, regardless +// of the zero-value for that type. +// This should only be used if absolutely required/needed. +func (d *ResourceData) GetOkRaw(key string) (interface{}, bool) { + r := d.getRaw(key, getSourceSet) + exists := r.Exists && !r.Computed + return r.Value, exists +} + func (d *ResourceData) getRaw(key string, level getSource) getResult { var parts []string if key != "" { diff --git a/helper/schema/resource_data_test.go b/helper/schema/resource_data_test.go index 615a0f7f7a..bb79f2bca9 100644 --- a/helper/schema/resource_data_test.go +++ b/helper/schema/resource_data_test.go @@ -1082,6 +1082,204 @@ func TestResourceDataGetOk(t *testing.T) { } } +func TestResourceDataGetOkRaw(t *testing.T) { + cases := []struct { + Name string + Schema map[string]*Schema + State *terraform.InstanceState + Diff *terraform.InstanceDiff + Key string + Value interface{} + Ok bool + }{ + /* + * Primitives + */ + { + Name: "string-literal-empty", + Schema: map[string]*Schema{ + "availability_zone": { + Type: TypeString, + Optional: true, + Computed: true, + ForceNew: true, + }, + }, + + State: nil, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "availability_zone": { + Old: "", + New: "", + }, + }, + }, + + Key: "availability_zone", + Value: "", + Ok: true, + }, + + { + Name: "string-computed-empty", + Schema: map[string]*Schema{ + "availability_zone": { + Type: TypeString, + Optional: true, + Computed: true, + ForceNew: true, + }, + }, + + State: nil, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "availability_zone": { + Old: "", + New: "", + NewComputed: true, + }, + }, + }, + + Key: "availability_zone", + Value: "", + Ok: false, + }, + + { + Name: "string-optional-computed-nil-diff", + Schema: map[string]*Schema{ + "availability_zone": { + Type: TypeString, + Optional: true, + Computed: true, + ForceNew: true, + }, + }, + + State: nil, + + Diff: nil, + + Key: "availability_zone", + Value: "", + Ok: false, + }, + + /* + * Lists + */ + + { + Name: "list-optional", + Schema: map[string]*Schema{ + "ports": { + Type: TypeList, + Optional: true, + Elem: &Schema{Type: TypeInt}, + }, + }, + + State: nil, + + Diff: nil, + + Key: "ports", + Value: []interface{}{}, + Ok: false, + }, + + /* + * Map + */ + + { + Name: "map-optional", + Schema: map[string]*Schema{ + "ports": { + Type: TypeMap, + Optional: true, + }, + }, + + State: nil, + + Diff: nil, + + Key: "ports", + Value: map[string]interface{}{}, + Ok: false, + }, + + /* + * Set + */ + + { + Name: "set-optional", + Schema: map[string]*Schema{ + "ports": { + Type: TypeSet, + Optional: true, + Elem: &Schema{Type: TypeInt}, + Set: func(a interface{}) int { return a.(int) }, + }, + }, + + State: nil, + + Diff: nil, + + Key: "ports", + Value: []interface{}{}, + Ok: false, + }, + + { + Name: "set-optional-key", + Schema: map[string]*Schema{ + "ports": { + Type: TypeSet, + Optional: true, + Elem: &Schema{Type: TypeInt}, + Set: func(a interface{}) int { return a.(int) }, + }, + }, + + State: nil, + + Diff: nil, + + Key: "ports.0", + Value: 0, + Ok: false, + }, + } + + for _, tc := range cases { + d, err := schemaMap(tc.Schema).Data(tc.State, tc.Diff) + if err != nil { + t.Fatalf("%s err: %s", tc.Name, err) + } + + v, ok := d.GetOkRaw(tc.Key) + if s, ok := v.(*Set); ok { + v = s.List() + } + + if !reflect.DeepEqual(v, tc.Value) { + t.Fatalf("Bad %s: \n%#v", tc.Name, v) + } + if ok != tc.Ok { + t.Fatalf("%s: expected ok: %t, got: %t", tc.Name, tc.Ok, ok) + } + } +} + func TestResourceDataTimeout(t *testing.T) { cases := []struct { Name string From 268138dbd49368dafe0af174bf1b8fe9c1812e53 Mon Sep 17 00:00:00 2001 From: Jake Champlin Date: Thu, 3 Aug 2017 12:05:19 -0400 Subject: [PATCH 2/4] Rename to GetOkExists --- helper/schema/resource_data.go | 4 ++-- helper/schema/resource_data_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/helper/schema/resource_data.go b/helper/schema/resource_data.go index fcb44a2f81..15aa0b5d49 100644 --- a/helper/schema/resource_data.go +++ b/helper/schema/resource_data.go @@ -104,7 +104,7 @@ func (d *ResourceData) GetOk(key string) (interface{}, bool) { return r.Value, exists } -// GetOkRaw returns the data for a given key and whether or not the key +// GetOkExists returns the data for a given key and whether or not the key // has been set to a non-zero value. This is only useful for determining // if boolean attributes have been set, if they are Optional but do not // have a Default value. @@ -114,7 +114,7 @@ func (d *ResourceData) GetOk(key string) (interface{}, bool) { // without a default, to fully check for a literal assignment, regardless // of the zero-value for that type. // This should only be used if absolutely required/needed. -func (d *ResourceData) GetOkRaw(key string) (interface{}, bool) { +func (d *ResourceData) GetOkExists(key string) (interface{}, bool) { r := d.getRaw(key, getSourceSet) exists := r.Exists && !r.Computed return r.Value, exists diff --git a/helper/schema/resource_data_test.go b/helper/schema/resource_data_test.go index bb79f2bca9..27041f0668 100644 --- a/helper/schema/resource_data_test.go +++ b/helper/schema/resource_data_test.go @@ -1082,7 +1082,7 @@ func TestResourceDataGetOk(t *testing.T) { } } -func TestResourceDataGetOkRaw(t *testing.T) { +func TestResourceDataGetOkExists(t *testing.T) { cases := []struct { Name string Schema map[string]*Schema @@ -1266,7 +1266,7 @@ func TestResourceDataGetOkRaw(t *testing.T) { t.Fatalf("%s err: %s", tc.Name, err) } - v, ok := d.GetOkRaw(tc.Key) + v, ok := d.GetOkExists(tc.Key) if s, ok := v.(*Set); ok { v = s.List() } From fa272e8c9c8820b21f0721e549c94ea499a825ee Mon Sep 17 00:00:00 2001 From: Jake Champlin Date: Thu, 3 Aug 2017 14:14:39 -0400 Subject: [PATCH 3/4] Add more specific exists tests --- helper/schema/resource_data_test.go | 45 +++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/helper/schema/resource_data_test.go b/helper/schema/resource_data_test.go index 27041f0668..fea3df2ed4 100644 --- a/helper/schema/resource_data_test.go +++ b/helper/schema/resource_data_test.go @@ -1258,6 +1258,51 @@ func TestResourceDataGetOkExists(t *testing.T) { Value: 0, Ok: false, }, + + { + Name: "bool-literal-empty", + Schema: map[string]*Schema{ + "availability_zone": { + Type: TypeBool, + Optional: true, + Computed: true, + ForceNew: true, + }, + }, + + State: nil, + Diff: nil, + + Key: "availability_zone", + Value: false, + Ok: false, + }, + + { + Name: "bool-literal-set", + Schema: map[string]*Schema{ + "availability_zone": { + Type: TypeBool, + Optional: true, + Computed: true, + ForceNew: true, + }, + }, + + State: nil, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "availability_zone": { + New: "true", + }, + }, + }, + + Key: "availability_zone", + Value: true, + Ok: true, + }, } for _, tc := range cases { From d969f97e730d527a568a0929ce52df70e062bca0 Mon Sep 17 00:00:00 2001 From: Jake Champlin Date: Thu, 3 Aug 2017 17:53:07 -0400 Subject: [PATCH 4/4] update tests --- helper/schema/resource_data_test.go | 43 +++++++++++++++++------------ 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/helper/schema/resource_data_test.go b/helper/schema/resource_data_test.go index fea3df2ed4..09aefb8f12 100644 --- a/helper/schema/resource_data_test.go +++ b/helper/schema/resource_data_test.go @@ -1271,11 +1271,18 @@ func TestResourceDataGetOkExists(t *testing.T) { }, State: nil, - Diff: nil, + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "availability_zone": { + Old: "", + New: "", + }, + }, + }, Key: "availability_zone", Value: false, - Ok: false, + Ok: true, }, { @@ -1305,23 +1312,25 @@ func TestResourceDataGetOkExists(t *testing.T) { }, } - for _, tc := range cases { - d, err := schemaMap(tc.Schema).Data(tc.State, tc.Diff) - if err != nil { - t.Fatalf("%s err: %s", tc.Name, err) - } + for i, tc := range cases { + t.Run(fmt.Sprintf("%d-%s", i, tc.Name), func(t *testing.T) { + d, err := schemaMap(tc.Schema).Data(tc.State, tc.Diff) + if err != nil { + t.Fatalf("%s err: %s", tc.Name, err) + } - v, ok := d.GetOkExists(tc.Key) - if s, ok := v.(*Set); ok { - v = s.List() - } + v, ok := d.GetOkExists(tc.Key) + if s, ok := v.(*Set); ok { + v = s.List() + } - if !reflect.DeepEqual(v, tc.Value) { - t.Fatalf("Bad %s: \n%#v", tc.Name, v) - } - if ok != tc.Ok { - t.Fatalf("%s: expected ok: %t, got: %t", tc.Name, tc.Ok, ok) - } + if !reflect.DeepEqual(v, tc.Value) { + t.Fatalf("Bad %s: \n%#v", tc.Name, v) + } + if ok != tc.Ok { + t.Fatalf("%s: expected ok: %t, got: %t", tc.Name, tc.Ok, ok) + } + }) } }