-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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.
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
Add VPN options and enable acceleration to aws_vpn_connection resource #14740
Add VPN options and enable acceleration to aws_vpn_connection resource #14740
Conversation
Not an official terraform developer just a public review comment - if you could add some tests as well that would probably be appreciated by the reviewers. Thanks for developing this and submitting it! |
This is missing options for:
It would be good to add those before merge |
@wimnat I don't see those options under https://docs.aws.amazon.com/cli/latest/reference/ec2/modify-vpn-tunnel-options.html. Are you referring to the InsideCIDR values? Those are already supported by terraform (https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpn_connection#tunnel1_inside_cidr). Or are you referring to something else? |
@dthvt They live under https://docs.aws.amazon.com/cli/latest/reference/ec2/modify-vpn-connection-options.html |
@wimnat Ah, I honestly wasn't even aware of those options. Thanks! |
Hi @MRinalducci! The PR is looking good. I'm working on reviewing it now, but it will also need acceptance tests to be added. Please let me know if you'll have time to add the tests. If not, I can take a look at handling that. It looks like #12218 has done some of the same work, but since those features are a subset of what you have here, I'd like to move forward here. There is an acceptance test in #12218 you can take a look at to get started. |
Hi @bill-rich! Thanks for reviewing this PR.
I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great. I've got a few suggestions, but mostly related to style tweaks. Thank you for taking the time to add all these options!
0f2e15e
to
3fada75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few small changes, and I think this is ready to go.
Nice! Thank you 👍 |
This has been released in version 3.22.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
This change has caused some |
@wayneworkman it seems to come from commit 4acc6d8 which implements the refresh/read of tunnel attributes. |
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! |
Community Note
Relates OR Closes #11584 , #14698 , #11135 and #12237
Release note for CHANGELOG:
Usage:
Output from acceptance testing: