core: Add `GetOkExists` schema function #15723

grubernaut · 2017-08-03T18:15:01Z

Adds GetOkExists 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=TestResourceDataGetOkExists"
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/08/03 14:13:30 Generated command/internal_plugin_list.go
go test -i ./helper/schema || exit 1
echo ./helper/schema | \
        xargs -t -n4 go test -run=TestResourceDataGetOkExists -timeout=60s -parallel=4
go test -run=TestResourceDataGetOkExists -timeout=60s -parallel=4 ./helper/schema 
ok      github.com/hashicorp/terraform/helper/schema    0.007s

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 ```

jbardin · 2017-08-03T19:44:18Z

helper/schema/resource_data_test.go

+		},
+
+		{
+			Name: "bool-literal-empty",


I think this is also one of the troublesome combinations, and we need the the test to look like string-literal-empty above, except with TypeBool. That will at least give us one other data point where GetOkExists differs from GetOk.

jbardin · 2017-08-03T19:46:56Z

helper/schema/resource_data_test.go

+		},
+	}
+
+	for _, tc := range cases {


Any new table-driven tests should be using subtests. As a rule of thumb, any old test that I touch gets converted too. This makes debugging changes much easier, since you can see all the case failures at once, rather than one at a time.

grubernaut · 2017-08-03T21:52:05Z

Updated:

$ make test TEST=./helper/schema TESTARGS="-v -run=TestResourceDataGetOkExists"                                                                                                              
==> Checking that code complies with gofmt requirements...                                                                                                                                   
go generate $(go list ./... | grep -v /terraform/vendor/)                                                                                                                                    
2017/08/03 17:52:50 Generated command/internal_plugin_list.go                                                                                                                                
go test -i ./helper/schema || exit 1                                                                                                                                                         
echo ./helper/schema | \                                                                                                                                                                     
        xargs -t -n4 go test -v -run=TestResourceDataGetOkExists -timeout=60s -parallel=4                                                                                                    
go test -v -run=TestResourceDataGetOkExists -timeout=60s -parallel=4 ./helper/schema                                                                                                         
=== RUN   TestResourceDataGetOkExists                                                                                                                                                        
=== RUN   TestResourceDataGetOkExists/0-string-literal-empty                                                                                                                                 
=== RUN   TestResourceDataGetOkExists/1-string-computed-empty                                                                                                                                
=== RUN   TestResourceDataGetOkExists/2-string-optional-computed-nil-diff                                                                                                                    
=== RUN   TestResourceDataGetOkExists/3-list-optional                                                                                                                                        
=== RUN   TestResourceDataGetOkExists/4-map-optional                                                                                                                                         
=== RUN   TestResourceDataGetOkExists/5-set-optional                                                                                                                                         
=== RUN   TestResourceDataGetOkExists/6-set-optional-key                                                                                                                                     
=== RUN   TestResourceDataGetOkExists/7-bool-literal-empty                                                                                                                                   
=== RUN   TestResourceDataGetOkExists/8-bool-literal-set
--- PASS: TestResourceDataGetOkExists (0.00s)
    --- PASS: TestResourceDataGetOkExists/0-string-literal-empty (0.00s)
    --- PASS: TestResourceDataGetOkExists/1-string-computed-empty (0.00s)
    --- PASS: TestResourceDataGetOkExists/2-string-optional-computed-nil-diff (0.00s)
    --- PASS: TestResourceDataGetOkExists/3-list-optional (0.00s)
    --- PASS: TestResourceDataGetOkExists/4-map-optional (0.00s)
    --- PASS: TestResourceDataGetOkExists/5-set-optional (0.00s)
    --- PASS: TestResourceDataGetOkExists/6-set-optional-key (0.00s)
    --- PASS: TestResourceDataGetOkExists/7-bool-literal-empty (0.00s)
    --- PASS: TestResourceDataGetOkExists/8-bool-literal-set (0.00s)
PASS
ok      github.com/hashicorp/terraform/helper/schema    0.005s

jbardin

LGTM!

jen20 · 2017-08-04T15:33:23Z

👍 This has been a thorn for provider writers for a long time - happy to see a straightforward fix!

Previously we had no way of determining if a boolean attribute was a literal `false` or a zero-value type of `false`. With the core enhancement added to Terraform via hashicorp/terraform#15723, we can now reliably check for a zero-type value of a boolean attribute, or a literal boolean `false` inside of schema. This allows the `associate_public_ip_address` to work as expected, when configuring an AWS Instance inside of a subnet that defaults to public address assignment, with a literal `false` for the `associate_public_ip_address` attribute. ``` $ make testacc TEST=./aws TESTARGS="-run=TestAccAWSInstance_explicitAssociatePublicAddress" ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSInstance_explicitAssociatePublicAddress -timeout 120m === RUN TestAccAWSInstance_explicitAssociatePublicAddress --- PASS: TestAccAWSInstance_explicitAssociatePublicAddress (113.04s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 113.045s ```

apparentlymart · 2017-08-08T00:36:15Z

This looks similar to what was discussed in #14139; note that I found a troublesome case there with an attribute that was already in state and then removed from config. I suspect (though didn't confirm) that this change would have the same limitation, but perhaps we can accept that edge case given that this should work as expected in the main case.

We'll just need to be on the look out for bugs of this sort:

An attribute is TypeBool with a default of true.
Resource is created with the value explicitly set to false.
User removes the argument from config altogether, expecting it to adopt the default of true in the next plan.
...but actually, it remains false because GetOkExists returned false, true.

jen20 · 2017-08-08T02:25:24Z

That's actually the same pathology I found when I implemented this a while back (never committed, obviously). I think it should be easy to reproduce in a test to verify - fortunately it should be clear from a plan that the default has not been adopted.

grubernaut · 2017-08-08T13:31:56Z

Hey @jen20 + @apparentlymart,

I was able to test the scenario listed above with a resource that was TypeBool and the default of true. Creating the resource with an explicit false works as expected, and removing the attribute from config caused the plan to pick up the correct Default value.

-/+ aws_instance.web (new resource required)
      jake_test_attribute:  "false" => "true" (forces new resource)

Working on translating this into an accurate unit test at the moment.

Previously we had no way of determining if a boolean attribute was a literal `false` or a zero-value type of `false`. With the core enhancement added to Terraform via hashicorp/terraform#15723, we can now reliably check for a zero-type value of a boolean attribute, or a literal boolean `false` inside of schema. This allows the `associate_public_ip_address` to work as expected, when configuring an AWS Instance inside of a subnet that defaults to public address assignment, with a literal `false` for the `associate_public_ip_address` attribute. ``` $ make testacc TEST=./aws TESTARGS="-run=TestAccAWSInstance_associatePublic_" ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSInstance_associatePublic_ -timeout 120m === RUN TestAccAWSInstance_associatePublic_defaultPrivate --- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (106.54s) === RUN TestAccAWSInstance_associatePublic_defaultPublic --- PASS: TestAccAWSInstance_associatePublic_defaultPublic (215.38s) === RUN TestAccAWSInstance_associatePublic_explicitPublic --- PASS: TestAccAWSInstance_associatePublic_explicitPublic (112.23s) === RUN TestAccAWSInstance_associatePublic_explicitPrivate --- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (224.96s) === RUN TestAccAWSInstance_associatePublic_overridePublic --- PASS: TestAccAWSInstance_associatePublic_overridePublic (117.78s) === RUN TestAccAWSInstance_associatePublic_overridePrivate --- PASS: TestAccAWSInstance_associatePublic_overridePrivate (112.48s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 889.375s ```

tmshn · 2017-08-14T12:52:37Z

So the problem is totally fixed?

As I looked through, the code of the function is same as what I wrote here.
So I'm worried about that the same problem might happen which @apparentlymart pointed out here:
#14139 (comment)

Previously we had no way of determining if a boolean attribute was a literal `false` or a zero-value type of `false`. With the core enhancement added to Terraform via hashicorp/terraform#15723, we can now reliably check for a zero-type value of a boolean attribute, or a literal boolean `false` inside of schema. This allows the `associate_public_ip_address` to work as expected, when configuring an AWS Instance inside of a subnet that defaults to public address assignment, with a literal `false` for the `associate_public_ip_address` attribute. ``` $ make testacc TEST=./aws TESTARGS="-run=TestAccAWSInstance_associatePublic_" ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSInstance_associatePublic_ -timeout 120m === RUN TestAccAWSInstance_associatePublic_defaultPrivate --- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (106.54s) === RUN TestAccAWSInstance_associatePublic_defaultPublic --- PASS: TestAccAWSInstance_associatePublic_defaultPublic (215.38s) === RUN TestAccAWSInstance_associatePublic_explicitPublic --- PASS: TestAccAWSInstance_associatePublic_explicitPublic (112.23s) === RUN TestAccAWSInstance_associatePublic_explicitPrivate --- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (224.96s) === RUN TestAccAWSInstance_associatePublic_overridePublic --- PASS: TestAccAWSInstance_associatePublic_overridePublic (117.78s) === RUN TestAccAWSInstance_associatePublic_overridePrivate --- PASS: TestAccAWSInstance_associatePublic_overridePrivate (112.48s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 889.375s ```

GitLab Slack service implementation requires terraform's ResourceData.GetOkExists method, because of the large number of boolean attributes. See: hashicorp/terraform#15723

ghost · 2020-04-07T02:34:31Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

grubernaut added 3 commits August 2, 2017 11:18

Rename to GetOkExists

268138d

Add more specific exists tests

fa272e8

grubernaut added core enhancement labels Aug 3, 2017

grubernaut requested review from mitchellh and jbardin August 3, 2017 18:15

jbardin requested changes Aug 3, 2017

View reviewed changes

update tests

d969f97

jbardin approved these changes Aug 3, 2017

View reviewed changes

grubernaut merged commit 3600f0b into master Aug 3, 2017

grubernaut deleted the f-get-boolean-non-default branch August 3, 2017 22:06

grubernaut mentioned this pull request Aug 3, 2017

r/aws_instance: Fix associate_public_ip_address hashicorp/terraform-provider-aws#1340

Merged

grubernaut mentioned this pull request Aug 4, 2017

aws_instance associate_public_ip_address hashicorp/terraform-provider-aws#227

Closed

apparentlymart mentioned this pull request Sep 8, 2017

Need a "GetExists" or similar for determining if a key is set in a configuration #5694

Closed

apparentlymart mentioned this pull request Mar 13, 2018

[WIP] helper/schema: GetOkExists always returns true on updates #17557

Closed

sophaskins mentioned this pull request Dec 18, 2019

docs for GetOkExists are misleading hashicorp/terraform-plugin-sdk#283

Closed

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Add `GetOkExists` schema function #15723

core: Add `GetOkExists` schema function #15723

grubernaut commented Aug 3, 2017

jbardin Aug 3, 2017

jbardin Aug 3, 2017

grubernaut commented Aug 3, 2017

jbardin left a comment

jen20 commented Aug 4, 2017

apparentlymart commented Aug 8, 2017 •

edited

Loading

jen20 commented Aug 8, 2017

grubernaut commented Aug 8, 2017

tmshn commented Aug 14, 2017

ghost commented Apr 7, 2020

core: Add GetOkExists schema function #15723

core: Add GetOkExists schema function #15723

Conversation

grubernaut commented Aug 3, 2017

jbardin Aug 3, 2017

Choose a reason for hiding this comment

jbardin Aug 3, 2017

Choose a reason for hiding this comment

grubernaut commented Aug 3, 2017

jbardin left a comment

Choose a reason for hiding this comment

jen20 commented Aug 4, 2017

apparentlymart commented Aug 8, 2017 • edited Loading

jen20 commented Aug 8, 2017

grubernaut commented Aug 8, 2017

tmshn commented Aug 14, 2017

ghost commented Apr 7, 2020

core: Add `GetOkExists` schema function #15723

core: Add `GetOkExists` schema function #15723

apparentlymart commented Aug 8, 2017 •

edited

Loading