Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

r/aws_vpn_gateway: Only validate Amazon-side ASN when creating new #5291

Merged
merged 3 commits into from
Jul 30, 2018
Merged

r/aws_vpn_gateway: Only validate Amazon-side ASN when creating new #5291

merged 3 commits into from
Jul 30, 2018

Conversation

ewbankkit
Copy link
Contributor

Fixes #5263.

Unit tests:

Running tool: /usr/local/go/bin/go test -timeout 30s github.com/terraform-providers/terraform-provider-aws/aws -run ^TestValidateAmazonSideAsn$

ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.011s
Success: Tests passed.

Acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGateway_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAwsDxGateway_basic -timeout 120m
=== RUN   TestAccAwsDxGateway_basic
--- PASS: TestAccAwsDxGateway_basic (38.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	38.137s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVpnGateway_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSVpnGateway_basic -timeout 120m
=== RUN   TestAccAWSVpnGateway_basic
--- PASS: TestAccAWSVpnGateway_basic (87.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	87.083s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVpnGateway_withAmazonSideAsnSetToState'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSVpnGateway_withAmazonSideAsnSetToState -timeout 120m
=== RUN   TestAccAWSVpnGateway_withAmazonSideAsnSetToState
--- PASS: TestAccAWSVpnGateway_withAmazonSideAsnSetToState (74.12s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	74.135s

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Jul 21, 2018
ewbankkit
ewbankkit commented Jul 21, 2018
View reviewed changes
aws/validators.go Outdated
@@ -1733,20 +1733,20 @@ func validateStringIn(validValues ...string) schema.SchemaValidateFunc {
}
}

func validateAmazonSideAsn(v interface{}, k string) (ws []string, errors []error) {
func validateAmazonSideAsn(v interface{}, k string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified signature so it can be called from CustomizeDiff and ValidateFunc.

@bflad bflad added bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service. labels Jul 25, 2018
bflad
bflad reviewed Jul 26, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes current acceptance testing but isn't able to be tested since the legacy ASNs cannot be created and imported. As noted in my concern below, I wonder if we should just allow the two ASNs with the previous validation function instead.

aws/resource_aws_vpn_gateway.go Outdated
if diff.Id() == "" {
// Only validate Amazon-side ASN for new resources.
// Imported resources may have legacy 7224 or 9059 ASNs.
if v, ok := diff.GetOk("amazon_side_asn"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding this correctly, this logic doesn't prevent the validation issue after creation where amazon_side_asn = 7224 is present in the configuration. Since it is technically valid, I think instead of adding all this workaround code that we should simply loosen our plan time validation to accept the two legacy values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can do that.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 26, 2018
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Jul 28, 2018
@ewbankkit
Copy link
Contributor Author

Split validateAmazonSideAsn into validateDxGatewayAmazonSideAsn and validateVpnGatewayAmazonSideAsn.

Unit tests:

$ go test -timeout 30s github.com/terraform-providers/terraform-provider-aws/aws -run ^TestValidateDxGatewayAmazonSideAsn$
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.009s
$ go test -timeout 30s github.com/terraform-providers/terraform-provider-aws/aws -run ^TestValidateVpnGatewayAmazonSideAsn$
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.009s

Acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGateway_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAwsDxGateway_basic -timeout 120m
=== RUN   TestAccAwsDxGateway_basic
--- PASS: TestAccAwsDxGateway_basic (36.29s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	36.300s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVpnGateway_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSVpnGateway_basic -timeout 120m
=== RUN   TestAccAWSVpnGateway_basic
--- PASS: TestAccAWSVpnGateway_basic (70.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	70.940s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSVpnGateway_withAmazonSideAsnSetToState'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSVpnGateway_withAmazonSideAsnSetToState -timeout 120m
=== RUN   TestAccAWSVpnGateway_withAmazonSideAsnSetToState
--- PASS: TestAccAWSVpnGateway_withAmazonSideAsnSetToState (40.17s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	40.187s

@ewbankkit
Copy link
Contributor Author

@bflad Ready to re-review.

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 30, 2018
@bflad bflad added this to the v1.30.0 milestone Jul 30, 2018
bflad
bflad approved these changes Jul 30, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

11 tests passed (all tests)
=== RUN   TestAccAwsDxGateway_basic
--- PASS: TestAccAwsDxGateway_basic (27.71s)
=== RUN   TestAccAwsDxGateway_importBasic
--- PASS: TestAccAwsDxGateway_importBasic (28.49s)
=== RUN   TestAccAWSVpnGateway_disappears
--- PASS: TestAccAWSVpnGateway_disappears (46.41s)
=== RUN   TestAccAWSVpnGateway_withAmazonSideAsnSetToState
--- PASS: TestAccAWSVpnGateway_withAmazonSideAsnSetToState (51.38s)
=== RUN   TestAccAWSVpnGateway_delete
--- PASS: TestAccAWSVpnGateway_delete (59.33s)
=== RUN   TestAccAWSVpnGateway_importBasic
--- PASS: TestAccAWSVpnGateway_importBasic (61.65s)
=== RUN   TestAccAWSVpnGateway_withAvailabilityZoneSetToState
--- PASS: TestAccAWSVpnGateway_withAvailabilityZoneSetToState (63.23s)
=== RUN   TestAccAWSVpnGateway_tags
--- PASS: TestAccAWSVpnGateway_tags (67.79s)
=== RUN   TestAccAWSVpnGateway_basic
--- PASS: TestAccAWSVpnGateway_basic (111.19s)
=== RUN   TestAccAWSVpnGateway_reattach
--- PASS: TestAccAWSVpnGateway_reattach (126.28s)
=== RUN   TestAccAwsDxGateway_importComplex
--- PASS: TestAccAwsDxGateway_importComplex (1023.41s)

@bflad bflad merged commit 0130ebf into hashicorp:master Jul 30, 2018
bflad added a commit that referenced this pull request Jul 30, 2018
@bflad
Update CHANGELOG for #5291
9ca23fd
@ewbankkit ewbankkit deleted the issue-5263 branch July 31, 2018 11:50
@bflad
Copy link
Contributor

bflad commented Aug 2, 2018

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

@ewbankkit ewbankkit mentioned this pull request Aug 3, 2018
Merged
@ghost
Copy link

ghost commented Apr 4, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.30.0
Development

Successfully merging this pull request may close these issues.

issue with vpn_gateway and amazon_side_asn
2 participants
@ewbankkit @bflad