Retry for deleting default DHCP options, plus pagination plus a test sweeper #8907

ryndaniels · 2019-06-07T15:24:42Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

References #7873

Release note for CHANGELOG:

BUG FIXES:
* resource/default_vpc_dhcp_options: Add pagination to get the default DHCP options correctly
* resource/vpc_dhcp_options: Add final retry to deleting DHCP options

Output from acceptance testing:

$ make testacc TESTARGS="-run=TestAccAWSDefaultVpcDhcpOptions"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSDefaultVpcDhcpOptions -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSDefaultVpcDhcpOptions_basic
=== PAUSE TestAccAWSDefaultVpcDhcpOptions_basic
=== CONT  TestAccAWSDefaultVpcDhcpOptions_basic
--- PASS: TestAccAWSDefaultVpcDhcpOptions_basic (20.89s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       22.668s


$ make testacc TESTARGS="-run=TestAccAWSDHCPOptions" ==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSDHCPOptions -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSDHCPOptionsAssociation_basic
=== PAUSE TestAccAWSDHCPOptionsAssociation_basic
=== RUN   TestAccAWSDHCPOptions_importBasic
=== PAUSE TestAccAWSDHCPOptions_importBasic
=== RUN   TestAccAWSDHCPOptions_basic
=== PAUSE TestAccAWSDHCPOptions_basic
=== RUN   TestAccAWSDHCPOptions_deleteOptions
=== PAUSE TestAccAWSDHCPOptions_deleteOptions
=== CONT  TestAccAWSDHCPOptionsAssociation_basic
=== CONT  TestAccAWSDHCPOptions_deleteOptions
=== CONT  TestAccAWSDHCPOptions_importBasic
=== CONT  TestAccAWSDHCPOptions_basic
--- PASS: TestAccAWSDHCPOptions_deleteOptions (19.86s)
--- PASS: TestAccAWSDHCPOptions_basic (23.54s)
--- PASS: TestAccAWSDHCPOptions_importBasic (25.10s)
--- PASS: TestAccAWSDHCPOptionsAssociation_basic (49.07s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       49.880s

…sweeper

nywilken

Nicely done!I left a few nits to help consolidate things a bit in the test sweeper, but otherwise good to go.

nywilken · 2019-06-07T15:54:33Z

aws/resource_aws_vpc_dhcp_options_test.go

+			var domainName string
+			var defaultDomainNameFound, defaultDomainNameServersFound bool
+
+			if region == "us-east-1" {


Nit lets start with domainName = region + ".compute.internal" then use the if to change the value is the condition region == "us-east-1" is true.

## remove 'var domainName string' on line #33 domainName := region + ".compute.internal" if region == "us-east-1" { domainName = "ec2.internal" }

nywilken · 2019-06-07T15:59:44Z

aws/resource_aws_vpc_dhcp_options_test.go

+				domainName = region + ".compute.internal"
+			}
+
+			for _, dhcpConfiguration := range dhcpOption.DhcpConfigurations {


Would be helpful to add a comment that here indicating that we are skipping the default DHCP options here.

nywilken · 2019-06-07T16:01:53Z

aws/resource_aws_vpc_dhcp_options_test.go

+
+			for _, dhcpConfiguration := range dhcpOption.DhcpConfigurations {
+				if aws.StringValue(dhcpConfiguration.Key) == "domain-name" {
+					if len(dhcpConfiguration.Values) == 0 || len(dhcpConfiguration.Values) > 1 {


Nit to help grok the conditional

Suggested change

if len(dhcpConfiguration.Values) == 0 || len(dhcpConfiguration.Values) > 1 {

if len(dhcpConfiguration.Values) != 1 || dhcpConfiguration.Values[0] == nil {

nywilken · 2019-06-07T16:02:57Z

aws/resource_aws_vpc_dhcp_options_test.go

+						continue
+					}
+
+					if dhcpConfiguration.Values[0] != nil && aws.StringValue(dhcpConfiguration.Values[0].Value) == domainName {


After applying the above you can change this to the following

Suggested change

if dhcpConfiguration.Values[0] != nil && aws.StringValue(dhcpConfiguration.Values[0].Value) == domainName {

if aws.StringValue(dhcpConfiguration.Values[0].Value) == domainName {

nywilken · 2019-06-07T16:03:39Z

aws/resource_aws_vpc_dhcp_options_test.go

+						defaultDomainNameFound = true
+					}
+				} else if aws.StringValue(dhcpConfiguration.Key) == "domain-name-servers" {
+					if len(dhcpConfiguration.Values) == 0 || len(dhcpConfiguration.Values) > 1 {


Same changes from above can be applied here.

nywilken · 2019-06-07T16:03:58Z

aws/resource_aws_vpc_dhcp_options_test.go

+						continue
+					}
+
+					if dhcpConfiguration.Values[0] != nil && aws.StringValue(dhcpConfiguration.Values[0].Value) == "AmazonProvidedDNS" {


ditto ☝️

ghost · 2019-11-03T15:23:01Z

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!

Retry for deleting default DHCP options, plus pagination plus a test …

d952370

…sweeper

ghost added size/M Managed by automation to categorize the size of a PR. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 7, 2019

ryndaniels added bug Addresses a defect in current functionality. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 7, 2019

ryndaniels requested a review from bflad June 7, 2019 15:25

nywilken reviewed Jun 7, 2019

View reviewed changes

nywilken self-assigned this Jun 7, 2019

Nits being picked

e054daf

ryndaniels merged commit a7b3917 into master Jun 13, 2019

ryndaniels added this to the v2.15.0 milestone Jun 13, 2019

ryndaniels deleted the rfd-dhcp-options branch June 20, 2019 11:54

ghost locked and limited conversation to collaborators Nov 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry for deleting default DHCP options, plus pagination plus a test sweeper #8907

Retry for deleting default DHCP options, plus pagination plus a test sweeper #8907

ryndaniels commented Jun 7, 2019

nywilken left a comment

nywilken Jun 7, 2019

nywilken Jun 7, 2019

nywilken Jun 7, 2019

nywilken Jun 7, 2019

nywilken Jun 7, 2019

nywilken Jun 7, 2019

ghost commented Nov 3, 2019

	if len(dhcpConfiguration.Values) == 0 \|\| len(dhcpConfiguration.Values) > 1 {
	if len(dhcpConfiguration.Values) != 1 \|\| dhcpConfiguration.Values[0] == nil {

	if dhcpConfiguration.Values[0] != nil && aws.StringValue(dhcpConfiguration.Values[0].Value) == domainName {
	if aws.StringValue(dhcpConfiguration.Values[0].Value) == domainName {

Retry for deleting default DHCP options, plus pagination plus a test sweeper #8907

Retry for deleting default DHCP options, plus pagination plus a test sweeper #8907

Conversation

ryndaniels commented Jun 7, 2019

Community Note

nywilken left a comment

Choose a reason for hiding this comment

nywilken Jun 7, 2019

Choose a reason for hiding this comment

nywilken Jun 7, 2019

Choose a reason for hiding this comment

nywilken Jun 7, 2019

Choose a reason for hiding this comment

nywilken Jun 7, 2019

Choose a reason for hiding this comment

nywilken Jun 7, 2019

Choose a reason for hiding this comment

nywilken Jun 7, 2019

Choose a reason for hiding this comment

ghost commented Nov 3, 2019