resource/aws_launch_template: Ensure ebs_optimized argument accepts "unspecified" value #5627

bjwschaap · 2018-08-21T14:11:11Z

First attempt to contribute, so any guidance is much appreciated.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLaunchTemplate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLaunchTemplate -timeout 120m
=== RUN   TestAccAWSLaunchTemplate_importBasic
--- PASS: TestAccAWSLaunchTemplate_importBasic (26.03s)
=== RUN   TestAccAWSLaunchTemplate_importData
--- PASS: TestAccAWSLaunchTemplate_importData (23.80s)
=== RUN   TestAccAWSLaunchTemplate_basic
--- PASS: TestAccAWSLaunchTemplate_basic (22.79s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS (27.85s)
=== RUN   TestAccAWSLaunchTemplate_data
--- PASS: TestAccAWSLaunchTemplate_data (20.99s)
=== RUN   TestAccAWSLaunchTemplate_update
--- PASS: TestAccAWSLaunchTemplate_update (85.40s)
=== RUN   TestAccAWSLaunchTemplate_tags
--- PASS: TestAccAWSLaunchTemplate_tags (41.26s)
=== RUN   TestAccAWSLaunchTemplate_nonBurstable
--- PASS: TestAccAWSLaunchTemplate_nonBurstable (21.50s)
=== RUN   TestAccAWSLaunchTemplate_networkInterface
--- PASS: TestAccAWSLaunchTemplate_networkInterface (56.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	326.306s

bflad · 2018-08-21T14:33:14Z

Hi @bjwschaap 👋 Thanks for submitting this.

Do we know for sure that the default for this attribute is false? Many of the options for EC2 Launch Templates can have three states for booleans: "unspecified" / false / true. The unspecified case usually is for designating that the AMI's setting applies to any instances launched (e.g. AMI has EBS optimized set to enabled so launching instances inherit this setting) while false / true are used to override the AMI setting.

blckct · 2018-08-21T15:01:46Z

It should default to unspecified so the instances that are ebs optimized by default launch with ebs optimization.

bflad · 2018-08-21T17:33:15Z

Its likely that this implementation should instead be similar to: #5632 where we use TypeString to allow "" in addition to true/false. @bjwschaap are you able to update this pull request?

bjwschaap · 2018-08-21T18:53:25Z

@bflad and @blckct thanks for the feedback! I can update the PR, no problem.

So as far as I can tell from the documentation the default state for a launch template on AWS is indeed "unspecified". I was too quick with jumping to a conclusion with this implementation, based on the conversation in the issue thread. My apologies..

Based on @bflad comment about #5632 I think I have enough pointers to improve this PR.

bjwschaap · 2018-08-21T21:34:46Z

@bflad @blckct I'm hitting an issue when testing. I'm probably missing something obvious here, but I'm a bit stuck at the moment. Maybe you have some pointers for me?

I've added ebs_optimized = false to testAccAWSLaunchTemplateConfig_data in the test. When running it fails because somehow it finds "0" (or "1" if I set ebs_optimized to true) instead of the string representation for the boolean:

=== RUN   TestAccAWSLaunchTemplate_importData
2018/08/21 23:13:45 [INFO] Test: Using us-west-2 as test region
2018/08/21 23:13:45 [INFO] No assume_role block read from configuration
2018/08/21 23:13:45 [INFO] Building AWS region structure
2018/08/21 23:13:45 [INFO] Building AWS auth structure
2018/08/21 23:13:45 [INFO] Setting AWS metadata API timeout to 100ms
2018/08/21 23:13:46 [INFO] Ignoring AWS metadata API endpoint at default location as it doesn't return any instance-id
2018/08/21 23:13:46 [INFO] AWS Auth provider used: "EnvProvider"
2018/08/21 23:13:46 [INFO] Initializing DeviceFarm SDK connection
2018/08/21 23:13:47 [INFO] terraform: building graph: GraphTypeValidate
2018/08/21 23:13:48 [ERROR] root: eval: *terraform.EvalValidateResource, err: Warnings: []. Errors: [expected ebs_optimized to be one of [ false true], got 0]
2018/08/21 23:13:48 [ERROR] root: eval: *terraform.EvalSequence, err: Warnings: []. Errors: [expected ebs_optimized to be one of [ false true], got 0]
2018/08/21 23:13:48 [WARN] Skipping destroy test since there is no state.
--- FAIL: TestAccAWSLaunchTemplate_importData (2.07s)
	testing.go:527: Step 0 error: config is invalid: aws_launch_template.foo: expected ebs_optimized to be one of [ false true], got 0

To my understanding resourceAwsLaunchTemplateRead is used to fetch the LaunchTemplate from EC2, and copy/transform the data from the ec2 API into the terraform schema (unmarshall). And buildLaunchTemplateData the other way around to convert the schema to an ec2 API struct (marshall). Are these assumptions correct? I've double checked that I'm converting the string to bool and back the correct way. So I'm not sure if my implementation is wrong, or the test is broken.

I'll commit and push what I have so far so you can take a look.

bflad · 2018-08-30T14:00:09Z

@bjwschaap I found the fix required for handling booleans in TypeString, you can setup this attribute to use the following DiffSuppressFunc and ValidateFunc:

"ebs_optimized": {
	// Use TypeString to allow an "unspecified" value,
	// since TypeBool only has true/false with false default.
	// The conversion from bare true/false values in
	// configurations to TypeString value is currently safe.
	Type:             schema.TypeString,
	Optional:         true,
	DiffSuppressFunc: suppressEquivalentTypeStringBoolean,
	ValidateFunc:     validateTypeStringNullableBoolean,
},

…form-provider-aws into fix_5458

bjwschaap · 2018-08-31T05:44:01Z

make testacc TEST=./aws TESTARGS='-run=TestAccAWSLaunchTemplate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLaunchTemplate -timeout 120m
=== RUN   TestAccAWSLaunchTemplate_importBasic
--- PASS: TestAccAWSLaunchTemplate_importBasic (24.31s)
=== RUN   TestAccAWSLaunchTemplate_importData
--- PASS: TestAccAWSLaunchTemplate_importData (22.24s)
=== RUN   TestAccAWSLaunchTemplate_basic
--- PASS: TestAccAWSLaunchTemplate_basic (20.45s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS (54.46s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination (83.05s)
=== RUN   TestAccAWSLaunchTemplate_data
--- PASS: TestAccAWSLaunchTemplate_data (20.53s)
=== RUN   TestAccAWSLaunchTemplate_update
--- PASS: TestAccAWSLaunchTemplate_update (80.70s)
=== RUN   TestAccAWSLaunchTemplate_tags
--- PASS: TestAccAWSLaunchTemplate_tags (37.72s)
=== RUN   TestAccAWSLaunchTemplate_nonBurstable
--- PASS: TestAccAWSLaunchTemplate_nonBurstable (19.63s)
=== RUN   TestAccAWSLaunchTemplate_networkInterface
--- PASS: TestAccAWSLaunchTemplate_networkInterface (56.29s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	419.418s

bjwschaap · 2018-08-31T05:44:31Z

Please review @bflad

bflad

Two more little things then I think we should be good to go 👍

bflad · 2018-08-31T14:43:53Z

aws/resource_aws_launch_template.go

@@ -549,6 +554,10 @@ func resourceAwsLaunchTemplateRead(d *schema.ResourceData, meta interface{}) err
 	d.Set("user_data", ltData.UserData)
 	d.Set("vpc_security_group_ids", aws.StringValueSlice(ltData.SecurityGroupIds))

+	if ltData.EbsOptimized != nil {


We should probably additionally call d.Set("ebs_optimized", "") before this if to handle ensure Terraform catches the scenario where the attribute is set to "unspecified" out of band.

bflad · 2018-08-31T14:48:02Z

aws/resource_aws_launch_template_test.go

@@ -122,6 +122,7 @@ func TestAccAWSLaunchTemplate_data(t *testing.T) {
 					resource.TestCheckResourceAttr(resName, "block_device_mappings.#", "1"),
 					resource.TestCheckResourceAttr(resName, "credit_specification.#", "1"),
 					resource.TestCheckResourceAttrSet(resName, "disable_api_termination"),
+					resource.TestCheckResourceAttrSet(resName, "ebs_optimized"),


Rather than just testing the attribute being set (to anything) we should ensure its value is correct:

resource.TestCheckResourceAttr(resName, "ebs_optimized", "false"),

We should also ensure at least one test whose configuration doesn't include ebs_optimized has the correct value as well:

resource.TestCheckResourceAttr(resName, "ebs_optimized", ""),

bjwschaap · 2018-09-06T12:48:13Z

@bflad Thnx for the feedback. Sorry for the late response on my behalf. I'll try and update with your review comments as soon as I can so we can get this shipped.

…form-provider-aws into fix_5458

bjwschaap · 2018-09-06T13:25:16Z

make testacc TEST=./aws TESTARGS='-run=TestAccAWSLaunchTemplate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLaunchTemplate -timeout 120m
=== RUN   TestAccAWSLaunchTemplate_importBasic
--- PASS: TestAccAWSLaunchTemplate_importBasic (24.47s)
=== RUN   TestAccAWSLaunchTemplate_importData
--- PASS: TestAccAWSLaunchTemplate_importData (22.26s)
=== RUN   TestAccAWSLaunchTemplate_basic
--- PASS: TestAccAWSLaunchTemplate_basic (21.73s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS (54.79s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination (84.47s)
=== RUN   TestAccAWSLaunchTemplate_data
--- PASS: TestAccAWSLaunchTemplate_data (20.63s)
=== RUN   TestAccAWSLaunchTemplate_update
--- PASS: TestAccAWSLaunchTemplate_update (93.52s)
=== RUN   TestAccAWSLaunchTemplate_tags
--- PASS: TestAccAWSLaunchTemplate_tags (39.46s)
=== RUN   TestAccAWSLaunchTemplate_nonBurstable
--- PASS: TestAccAWSLaunchTemplate_nonBurstable (21.12s)
=== RUN   TestAccAWSLaunchTemplate_networkInterface
--- PASS: TestAccAWSLaunchTemplate_networkInterface (56.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	439.478s

bflad

Looks great! Thanks, @bjwschaap! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAWSLaunchTemplate_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLaunchTemplate_ -timeout 120m
=== RUN   TestAccAWSLaunchTemplate_importBasic
--- PASS: TestAccAWSLaunchTemplate_importBasic (15.93s)
=== RUN   TestAccAWSLaunchTemplate_importData
--- PASS: TestAccAWSLaunchTemplate_importData (14.00s)
=== RUN   TestAccAWSLaunchTemplate_basic
--- PASS: TestAccAWSLaunchTemplate_basic (12.35s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS (50.21s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination (40.73s)
=== RUN   TestAccAWSLaunchTemplate_data
--- PASS: TestAccAWSLaunchTemplate_data (11.30s)
=== RUN   TestAccAWSLaunchTemplate_update
--- PASS: TestAccAWSLaunchTemplate_update (46.02s)
=== RUN   TestAccAWSLaunchTemplate_tags
--- PASS: TestAccAWSLaunchTemplate_tags (23.07s)
=== RUN   TestAccAWSLaunchTemplate_nonBurstable
--- PASS: TestAccAWSLaunchTemplate_nonBurstable (13.67s)
=== RUN   TestAccAWSLaunchTemplate_networkInterface
--- PASS: TestAccAWSLaunchTemplate_networkInterface (33.65s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	261.728s

bflad · 2018-09-13T02:54:04Z

This has been released in version 1.36.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-03T17:37:51Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Make ebs_optimized default False

95cdc94

ghost added the size/XS Managed by automation to categorize the size of a PR. label Aug 21, 2018

bflad added bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service. labels Aug 21, 2018

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 21, 2018

bjwschaap changed the title ~~Make ebs_optimized default False~~ [WIP] Make ebs_optimized default False Aug 21, 2018

Refactor ebs_optimized to schema.TypeString

6f641fb

ghost added size/S Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Aug 21, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Aug 22, 2018

bjwschaap added 2 commits August 31, 2018 07:14

Merge branch 'master' of https://github.com/terraform-providers/terra…

99b3255

…form-provider-aws into fix_5458

Add TypeStringBoolean equivalence from hashicorp#5632

eeb04c6

bjwschaap changed the title ~~[WIP] Make ebs_optimized default False~~ Make ebs_optimized default False Aug 31, 2018

bflad reviewed Aug 31, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 4, 2018

bjwschaap added 2 commits September 6, 2018 14:52

Merge branch 'master' of https://github.com/terraform-providers/terra…

8b06148

…form-provider-aws into fix_5458

Changes w/r to review comments

75d3eb5

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 6, 2018

bflad added this to the v1.36.0 milestone Sep 6, 2018

bflad approved these changes Sep 6, 2018

View reviewed changes

bflad changed the title ~~Make ebs_optimized default False~~ resource/aws_launch_template: Ensure ebs_optimized argument accepts "unspecified" value Sep 6, 2018

bflad merged commit 2192c13 into hashicorp:master Sep 6, 2018

bflad added a commit that referenced this pull request Sep 6, 2018

Update CHANGELOG for #5627

130774c

bjwschaap deleted the fix_5458 branch September 7, 2018 06:21

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_launch_template: Ensure ebs_optimized argument accepts "unspecified" value #5627

resource/aws_launch_template: Ensure ebs_optimized argument accepts "unspecified" value #5627

bjwschaap commented Aug 21, 2018

bflad commented Aug 21, 2018

blckct commented Aug 21, 2018

bflad commented Aug 21, 2018

bjwschaap commented Aug 21, 2018 •

edited

Loading

bjwschaap commented Aug 21, 2018

bflad commented Aug 30, 2018

bjwschaap commented Aug 31, 2018

bjwschaap commented Aug 31, 2018

bflad left a comment

bflad Aug 31, 2018

bflad Aug 31, 2018

bjwschaap commented Sep 6, 2018

bjwschaap commented Sep 6, 2018

bflad left a comment

bflad commented Sep 13, 2018

ghost commented Apr 3, 2020

resource/aws_launch_template: Ensure ebs_optimized argument accepts "unspecified" value #5627

resource/aws_launch_template: Ensure ebs_optimized argument accepts "unspecified" value #5627

Conversation

bjwschaap commented Aug 21, 2018

bflad commented Aug 21, 2018

blckct commented Aug 21, 2018

bflad commented Aug 21, 2018

bjwschaap commented Aug 21, 2018 • edited Loading

bjwschaap commented Aug 21, 2018

bflad commented Aug 30, 2018

bjwschaap commented Aug 31, 2018

bjwschaap commented Aug 31, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Aug 31, 2018

Choose a reason for hiding this comment

bflad Aug 31, 2018

Choose a reason for hiding this comment

bjwschaap commented Sep 6, 2018

bjwschaap commented Sep 6, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Sep 13, 2018

ghost commented Apr 3, 2020

bjwschaap commented Aug 21, 2018 •

edited

Loading