New Resource/Data Source: `azurerm_nat_gateway` and `azurerm_subnet_nat_gateway_association` #4449

ArcturusZhang · 2019-09-27T09:22:28Z

Added new resource and data source of nat-gateway.

azurerm/resource_arm_subnet.go

WodansSon

@ArcturusZhang , Thanks for the PR and it's looking really good. I left some comments asking a few questions and requesting a few, mostly minor, changes. Thanks for the PR again, this is great! 🐯

azurerm/data_source_nat_gateway.go

azurerm/internal/services/network/client.go

azurerm/resource_arm_nat_gateway.go

azurerm/resource_arm_nat_gateway_test.go

website/docs/r/nat_gateway.html.markdown

WodansSon

@ArcturusZhang thanks for the updates, looking really good! Very close, left a few more comments, mostly around documentation. 🚀

WodansSon · 2019-10-21T19:12:52Z

azurerm/resource_arm_nat_gateway.go

+func validateNameForNatGateway(i interface{}, k string) ([]string, []error) {
+	v, ok := i.(string)
+	if !ok {
+		return nil, []error{fmt.Errorf("expected type of %q to be string", k)}
+	}
+	// test length
+	if l := len(v); l < 1 || l > 80 {
+		return nil, []error{fmt.Errorf("length of field name must be between 1 and 80 characters")}
+	}
+	// regex test:
+	// The name must begin with a letter or number, end with a letter, number or underscore,
+	// and may contain only letters, numbers, underscores, periods, or hyphens.
+	regex := regexp.MustCompile(`^[a-zA-Z0-9](([\w-.]*)\w)?$`)
+	if ok := regex.MatchString(v); !ok {
+		return nil, []error{fmt.Errorf("the name must begin with a letter or number, end with a letter, number or underscore, and may contain only letters, numbers, underscores, periods, or hyphens")}
+	}
+	return nil, nil
+}


I too have implemented this validation function in my GenericRFC3986Compliance PR... While it's fine to put the validation code in the resource itself, I find it a bit nicer to create a validation.go file in the .\services\network directory. That way we keep all the package specific stuff in the same spot in the source tree. I would also pick another var for the length test as l and 1 look almost identical and makes it hard from a code readability point of view, maybe us a t for test instead? Maybe if the PR gets merged we can use the GenericRFC3986Compliance function?

I could rebase this pr to your branch, and then use the validation function there. Does this make sense?

azurerm/resource_arm_subnet_nat_gateway_association_test.go

website/docs/d/nat_gateway.html.markdown

website/docs/r/nat_gateway.html.markdown

ArcturusZhang · 2019-10-22T07:46:20Z

I made some changes, please have a look again

katbyte

Thanks for the revisions @ArcturusZhang,

I've left some comments inline, nothing to major. I would like to see more whitespace and newlines in functions as when there is none it is much harder to read, review, and maintain. Thanks!

azurerm/data_source_nat_gateway.go

website/docs/r/nat_gateway.html.markdown

katbyte · 2019-10-22T14:36:57Z

website/docs/r/nat_gateway.html.markdown

+
+* `public_ip_prefix_ids` - (Optional) A list of existing `azurerm_public_ip_prefix` resource IDs.
+
+* `resource_guid` - (Optional) The resource GUID property of the Nat Gateway.


Could we add a little more details here what the property does, not just what it is?

This field is computed, maybe I should remove this from this doc?

you have it as optional/computed?

then it should be moved down to attributes exported

Moved to Attributes Reference section

website/docs/r/nat_gateway.html.markdown

azurerm/resource_arm_nat_gateway.go

ArcturusZhang · 2019-10-23T05:15:24Z

Hi @WodansSon @katbyte I made some changes and left some comments, please have a look again.
And I may need some hint about the comment in data source doc, please see the comments for detail. Thanks!

katbyte

Hi @ArcturusZhang,

Thank you for making those changes. I've re-reviewed and left some more comments inline. I also ran the tests which are failing with:

------- Stdout: -------
=== RUN   TestAccAzureRMNatGateway_basic
=== PAUSE TestAccAzureRMNatGateway_basic
=== CONT  TestAccAzureRMNatGateway_basic

------- Stderr: -------
2019/11/01 00:52:36 [DEBUG] Registering Data Sources for "Compute"..
2019/11/01 00:52:36 [DEBUG] Registering Resources for "Compute"..
panic: interface conversion: interface {} is *schema.Set, not []interface {}

azurerm/data_source_nat_gateway.go

azurerm/resource_arm_nat_gateway.go

katbyte · 2019-11-01T00:59:11Z

website/docs/r/nat_gateway.html.markdown

+
+* `public_ip_prefix_ids` - (Optional) A list of existing `azurerm_public_ip_prefix` resource IDs.
+
+* `resource_guid` - (Optional) The resource GUID property of the Nat Gateway.


you have it as optional/computed?

then it should be moved down to attributes exported

website/docs/r/nat_gateway.html.markdown

website/docs/d/nat_gateway.html.markdown

website/docs/r/nat_gateway.html.markdown

azurerm/resource_arm_subnet_nat_gateway_association.go

WodansSon

I have made a few changes to the code, this LGTM now! Thanks! 🚀

ArcturusZhang · 2019-12-04T06:05:03Z

I have made a few changes to the code, this LGTM now! Thanks! 🚀

hooray! thanks!

katbyte

Thanks for the updates @ArcturusZhang & @WodansSon. I've given this one final review and noticed a couple things left. It also appears there are no docs for the association resource?

azurerm/data_source_nat_gateway_test.go

azurerm/resource_arm_nat_gateway.go

azurerm/resource_arm_nat_gateway_test.go

katbyte · 2019-12-04T22:20:33Z

website/docs/r/nat_gateway.html.markdown

+
+Manages an Azure NAT Gateway instance.
+
+-> **NOTE:** The Azure NAT Gateway service is currently in private preview. Your subscription must be on the NAT Gateway private preview whitelist for this resource to be provisioned correctly. If you attempt to provision this resource and receive an `InvalidResourceType` error that means that your subscription is not part of the NAT Gateway private preview whitelist and you will not be able to use this resource. The NAT Gateway private preview service is currently only available in the `East US 2` and `West Central US` regions.


Could we include warnings about possible breaking changes & a note to contact azure support if a user wants to participate?

Added some more information, please have a look

website/azurerm.erb

ArcturusZhang · 2019-12-05T03:10:20Z

Hi @katbyte I have made some changes, please have a look 🚀

…orm-provider-azurerm into nat-gateway

katbyte

Thanks for the revisions all, LGTM 👍

ghost · 2019-12-07T00:15:59Z

This has been released in version 1.38.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.38.0"
}
# ... other configuration ...

ghost · 2020-01-04T14:50:23Z

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

ghost added size/XXL documentation labels Sep 27, 2019

tombuildsstuff reviewed Sep 30, 2019

View reviewed changes

azurerm/resource_arm_subnet.go Outdated Show resolved Hide resolved

ArcturusZhang requested a review from tombuildsstuff October 1, 2019 06:48

ArcturusZhang changed the title ~~New Resource/Data Source: azurerm_nat_gateway and expose in azurerm_subnet~~ New Resource/Data Source: azurerm_nat_gateway and azurerm_subnet_nat_gateway_association Oct 1, 2019

WodansSon force-pushed the nat-gateway branch from 8385b2e to 67c280c Compare October 1, 2019 20:06

ArcturusZhang force-pushed the nat-gateway branch from 79ac450 to 5cc4a0b Compare October 3, 2019 04:46

ArcturusZhang force-pushed the nat-gateway branch 3 times, most recently from c1a79fd to 0e30cb4 Compare October 15, 2019 03:28

WodansSon added new-data-source new-resource service/network-interfaces labels Oct 16, 2019

WodansSon added this to the v1.37.0 milestone Oct 16, 2019

WodansSon requested changes Oct 16, 2019

View reviewed changes

ArcturusZhang requested a review from WodansSon October 16, 2019 02:44

WodansSon requested changes Oct 21, 2019

View reviewed changes

ArcturusZhang requested a review from WodansSon October 22, 2019 02:32

katbyte requested changes Oct 22, 2019

View reviewed changes

WodansSon requested changes Oct 23, 2019

View reviewed changes

azurerm/resource_arm_nat_gateway.go Outdated Show resolved Hide resolved

azurerm/resource_arm_nat_gateway.go Outdated Show resolved Hide resolved

ArcturusZhang requested review from katbyte and WodansSon October 23, 2019 05:26

tombuildsstuff removed this from the v1.37.0 milestone Oct 31, 2019

katbyte self-assigned this Oct 31, 2019

katbyte requested changes Nov 1, 2019

View reviewed changes

katbyte added the waiting-response label Nov 1, 2019

ArcturusZhang force-pushed the nat-gateway branch 2 times, most recently from 9d66799 to 1092e85 Compare November 1, 2019 05:30

WodansSon approved these changes Dec 4, 2019

View reviewed changes

katbyte requested changes Dec 4, 2019

View reviewed changes

katbyte modified the milestones: v1.39.0, v1.38.0 Dec 4, 2019

ArcturusZhang and others added 9 commits December 5, 2019 11:05

Add nat-gateway and subnet-nat-gateway-association

2698122

Resolve comments

8d7a841

Fix CI

7bf8142

Rebased to master and update api-version of network package

573b7c4

Rebased to master again

8183da0

Resolved comments and fix bad tests

cd658cb

Adding more information to doc

d04ec02

Update test location to use alt location

9725cd1

Resolving comments

dc970e5

ArcturusZhang force-pushed the nat-gateway branch from 56c9b8d to dc970e5 Compare December 5, 2019 03:05

Change comments in test to clarify the context of using alt location

9d69ef7

ArcturusZhang and others added 6 commits December 5, 2019 11:35

More official note

8f695e1

More refine on private preview note

de571fe

Merge branch 'nat-gateway' of https://github.com/ArcturusZhang/terraf…

b63b48e

…orm-provider-azurerm into nat-gateway

Remove duplicate func

b591f59

Update per PR comments

3e8f457

Updates per PR review

7dbafca

katbyte approved these changes Dec 5, 2019

View reviewed changes

katbyte merged commit ae2e3f8 into hashicorp:master Dec 5, 2019

katbyte added a commit that referenced this pull request Dec 5, 2019

Update CHANGELOG.md to include #4449

805022d

ArcturusZhang deleted the nat-gateway branch December 5, 2019 08:23

ghost locked and limited conversation to collaborators Jan 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource/Data Source: `azurerm_nat_gateway` and `azurerm_subnet_nat_gateway_association` #4449

New Resource/Data Source: `azurerm_nat_gateway` and `azurerm_subnet_nat_gateway_association` #4449

ArcturusZhang commented Sep 27, 2019

WodansSon left a comment

WodansSon left a comment

WodansSon Oct 21, 2019

ArcturusZhang Oct 22, 2019

ArcturusZhang commented Oct 22, 2019

katbyte left a comment

katbyte Oct 22, 2019

ArcturusZhang Oct 23, 2019 •

edited

Loading

katbyte Nov 1, 2019

ArcturusZhang Nov 1, 2019

ArcturusZhang commented Oct 23, 2019

katbyte left a comment

katbyte Nov 1, 2019

WodansSon left a comment

ArcturusZhang commented Dec 4, 2019

katbyte left a comment

katbyte Dec 4, 2019

ArcturusZhang Dec 5, 2019

ArcturusZhang commented Dec 5, 2019

katbyte left a comment

ghost commented Dec 7, 2019

ghost commented Jan 4, 2020


		* `public_ip_prefix_ids` - (Optional) A list of existing `azurerm_public_ip_prefix` resource IDs.

		* `resource_guid` - (Optional) The resource GUID property of the Nat Gateway.


		Manages an Azure NAT Gateway instance.

		-> NOTE: The Azure NAT Gateway service is currently in private preview. Your subscription must be on the NAT Gateway private preview whitelist for this resource to be provisioned correctly. If you attempt to provision this resource and receive an `InvalidResourceType` error that means that your subscription is not part of the NAT Gateway private preview whitelist and you will not be able to use this resource. The NAT Gateway private preview service is currently only available in the `East US 2` and `West Central US` regions.

New Resource/Data Source: azurerm_nat_gateway and azurerm_subnet_nat_gateway_association #4449

New Resource/Data Source: azurerm_nat_gateway and azurerm_subnet_nat_gateway_association #4449

Conversation

ArcturusZhang commented Sep 27, 2019

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang commented Oct 22, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang commented Oct 23, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

ArcturusZhang commented Dec 4, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang commented Dec 5, 2019

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Dec 7, 2019

ghost commented Jan 4, 2020

New Resource/Data Source: `azurerm_nat_gateway` and `azurerm_subnet_nat_gateway_association` #4449

New Resource/Data Source: `azurerm_nat_gateway` and `azurerm_subnet_nat_gateway_association` #4449

ArcturusZhang Oct 23, 2019 •

edited

Loading