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.

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

azurerm_vpn_gateway_nat_rule - support for port_range #16724

Merged

Conversation

neil-yechenwei
Copy link
Contributor

This PR is to support new property port_range for azurerm_vpn_gateway_nat_rule.

--- PASS: TestAccVpnGatewayNatRule_basic (4465.47s)
--- PASS: TestAccVpnGatewayNatRule_complete (4610.49s)
--- PASS: TestAccVpnGatewayNatRule_update (4727.26s)
--- PASS: TestAccVpnGatewayNatRule_externalMappingAndInternalMapping (4739.50s)
--- PASS: TestAccVpnGatewayNatRule_requiresImport (4805.02s)

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @neil-yechenwei, this is nearly there. Just a couple tweaks and it's good

Type: pluginsdk.TypeList,
Optional: true,
Computed: !features.FourPointOhBeta(),
AtLeastOneOf: func() []string {
Copy link
Member

Choose a reason for hiding this comment

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

All of AtLeastOneOf should be swapped to ExactlyOneOf since it's required and we don't want someone to put both external_address_space_mappings and external_mapping in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -58,16 +64,40 @@ The following arguments are supported:

* `vpn_gateway_id` - (Required) The ID of the VPN Gateway that this VPN Gateway NAT Rule belongs to. Changing this forces a new resource to be created.

* `external_address_space_mappings` - (Required) A list of CIDR Ranges which are used for external mapping of the VPN Gateway NAT Rule.
* `external_address_space_mappings` - (Optional) A list of CIDR Ranges which are used for external mapping of the VPN Gateway NAT Rule.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `external_address_space_mappings` - (Optional) A list of CIDR Ranges which are used for external mapping of the VPN Gateway NAT Rule.
* `external_address_space_mappings` - (Deprecated) A list of CIDR Ranges which are used for external mapping of the VPN Gateway NAT Rule.

Copy link
Member

Choose a reason for hiding this comment

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

Let's also move external_address_space_mappings and internal_address_space_mappings to the bottom to get them out of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


~> **NOTE:** `external_address_space_mappings` is deprecated and will be removed in favour of the property `external_mapping` in version 4.0 of the AzureRM Provider.

* `external_mapping` - (Optional) One or more `external_mapping` blocks as documented below.
Copy link
Member

Choose a reason for hiding this comment

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

Let's move external_mapping and internal_mapping up and make them required as we want those to be what people use going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


* `internal_address_space_mappings` - (Required) A list of CIDR Ranges which are used for internal mapping of the VPN Gateway NAT Rule.
* `internal_address_space_mappings` - (Optional) A list of CIDR Ranges which are used for internal mapping of the VPN Gateway NAT Rule.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `internal_address_space_mappings` - (Optional) A list of CIDR Ranges which are used for internal mapping of the VPN Gateway NAT Rule.
* `internal_address_space_mappings` - (Deprecated) A list of CIDR Ranges which are used for internal mapping of the VPN Gateway NAT Rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@neil-yechenwei
Copy link
Contributor Author

@mbfrahry , thanks for your comments. I've updated code. Please take another look. Thanks in advance.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbfrahry mbfrahry changed the title azurerm_vpn_gateway_nat_rule - support new property port_range azurerm_vpn_gateway_nat_rule - support for port_range Jun 2, 2022
@mbfrahry mbfrahry added this to the v3.9.0 milestone Jun 2, 2022
@mbfrahry mbfrahry merged commit 1348062 into hashicorp:main Jun 2, 2022
mbfrahry added a commit that referenced this pull request Jun 2, 2022
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

This functionality has been released in v3.9.0 of the Terraform 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. Thank you!

@github-actions
Copy link

github-actions bot commented Jul 4, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants