Add New Resource & Data Source: azurerm_netapp_volume #4933

neil-yechenwei · 2019-11-20T15:09:23Z

This PR is code implement for issue #3936

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments inline but this is off to a good start - there's a few bits of the schema which need to be updated (in particular the allowed_clients field), but if we can fix up the comments then we should be able to run the tests and take another look :)

Thanks!

tombuildsstuff · 2019-11-20T15:32:51Z

azurerm/data_source_netapp_volume.go

+				Computed: true,
+			},
+
+			"export_policy_rule": {


I think we can remove this field - since I don't think it's likely to be used by users?

I don't think so. Now both powershell and azure portal for netapp volume have opened this entry to let user edit export policy. And we cannot ensure the users won't use this, right? If they want to use, then I need to submit another PR for this? I think it's not a good option,right? So why don't we leave it as optional field? And this field should be optional after checked official doc https://docs.microsoft.com/en-us/azure/azure-netapp-files/azure-netapp-files-configure-export-policy.

whilst users may configure this on the Resource, the Data Source is Read Only and I can't envisage a scenario where users are likely to use this in the Data Source (since this needs to be a Set too) - for now let's remove it and we can add it back in if users request it

azurerm/data_source_netapp_volume.go

azurerm/internal/services/netapp/validate.go

azurerm/resource_arm_netapp_volume.go

website/docs/r/netapp_volume.html.markdown

tombuildsstuff · 2019-11-20T15:54:17Z

azurerm/resource_arm_netapp_volume.go

+			},
+
+			"export_policy_rule": {
+				Type:     schema.TypeList,


since the rule_index is part of this presumably these could be returned in any order and export_policy_rule needs to become a Set?

I found other field *_rule also is set as TypeList in existing resources of terraform. So I also can set this field as TypeList like others, right? I think even if these could be returned in any order, but I think it doesn't mean it must be set as TypeSet, right? For more details about rule_index and export_policy, please see this doc https://docs.microsoft.com/en-us/azure/azure-netapp-files/azure-netapp-files-configure-export-policy

Most of those should probably be TypeSets too, as order doesn't matter if rule_index is used, otherwise if we keep it as a typelist the ordering could be implicitly inferred by the index. WDYT @tombuildsstuff ?

azurerm/resource_arm_netapp_volume.go

tombuildsstuff

hi @neil-yechenwei

Thanks for this PR - I've taken a look through and left some comments inline but this is off to a good start - there's a few bits of the schema which need to be updated (in particular the allowed_clients field), but if we can fix up the comments then we should be able to run the tests and take another look :)

Thanks!

tombuildsstuff · 2019-11-20T15:59:12Z

(sorry for the double-review, Github returned a 500 🙃)

neil-yechenwei · 2019-11-21T14:57:02Z

@tombuildsstuff , Thanks for your comments. I've updated code per your comments.

katbyte

thanks for the revisions @neil-yechenwei, i've left some more comments inline

azurerm/resource_arm_netapp_pool_test.go

azurerm/resource_arm_netapp_volume.go

katbyte · 2019-11-22T01:28:30Z

azurerm/resource_arm_netapp_volume.go

+			},
+
+			"export_policy_rule": {
+				Type:     schema.TypeList,


Most of those should probably be TypeSets too, as order doesn't matter if rule_index is used, otherwise if we keep it as a typelist the ordering could be implicitly inferred by the index. WDYT @tombuildsstuff ?

azurerm/resource_arm_netapp_volume.go

azurerm/resource_arm_netapp_volume_test.go

website/docs/d/netapp_volume.html.markdown

website/docs/r/netapp_volume.html.markdown

neil-yechenwei · 2019-11-22T07:47:20Z

@katbyte , Thanks for your comments. I've updated code per your comments.

katbyte

Thanks for the revisions @neil-yechenwei, i've left a couple of additional minor comments inline

azurerm/resource_arm_netapp_pool.go

katbyte · 2019-12-03T09:50:27Z

azurerm/resource_arm_netapp_volume.go

+							ValidateFunc: validation.IntBetween(1, 5),
+						},
+						"allowed_clients": {
+							Type:     schema.TypeList,


This should probably be a set too>

azurerm/resource_arm_netapp_volume.go

katbyte · 2019-12-03T09:52:47Z

website/docs/d/netapp_volume.html.markdown

+
+* `location` - The Azure Region where the NetApp Volume exists.
+
+* `creation_token` - A unique file path for the volume.


Could we use data source language here

website/docs/d/netapp_volume.html.markdown

katbyte · 2019-12-03T09:54:01Z

website/docs/r/netapp_volume.html.markdown

+
+* `rule_index` - (Required) The index number of the rule.
+
+* `allowed_clients` - (Required) Client ingress specification as list with IPv4 CIDRs, IPv4 host addresses.


Could we fix the grammar here?

katbyte · 2019-12-03T09:54:14Z

website/docs/r/netapp_volume.html.markdown

+
+* `service_level` - (Required) The service level of the file system. Valid values include `Premium`, `Standard`, or `Ultra`.
+
+* `subnet_id` - (Required) The ID of a Subnet in which the NetApp Volume, which must have the delegation Microsoft.NetApp/volumes. Changing this forces a new resource to be created.


Could we fix the grammar here?

katbyte · 2019-12-03T09:54:43Z

website/docs/r/netapp_volume.html.markdown

+
+* `creation_token` - (Required) A unique file path for the volume. Used when creating mount targets.
+
+* `service_level` - (Required) The service level of the file system. Valid values include `Premium`, `Standard`, or `Ultra`.


Could we elaborate on what the service level means?

website/docs/r/netapp_volume.html.markdown

neil-yechenwei · 2019-12-04T04:42:44Z

Thanks for the comments @katbyte , I've created a new issue on azure-sdk-for-go for this API and updated the code per your comments.

katbyte

Thank you for the revisions @neil-yechenwei, I've left some more comments inline that should be addressed before merge.

azurerm/data_source_netapp_volume.go

azurerm/resource_arm_netapp_volume.go

katbyte · 2019-12-11T02:43:20Z

azurerm/resource_arm_netapp_volume.go

+		d.Set("subnet_id", props.SubnetID)
+
+		if props.UsageThreshold != nil {
+			d.Set("usage_threshold", *props.UsageThreshold/1073741824)


what units is usage_threshold in? could we make the name usage_threshold_in_units

I am confused. You want this field name is updated to "usage_threshold_in_units" or "storage_quota_in_gb"? I think "storage_quota_in_gb" as you suggested below is preferred so that I updated this field name to "storage_quota_in_gb".

website/docs/r/netapp_volume.html.markdown

katbyte · 2019-12-11T02:50:21Z

website/docs/r/netapp_volume.html.markdown

+
+* `account_name` - (Required) The name of the NetApp account in which the NetApp Pool should be created. Changing this forces a new resource to be created.
+
+* `creation_token` - (Required) A unique file path for the volume. Used when creating mount targets. Changing this forces a new resource to be created.


Should this perhaps be called something different? creation token seems odd if its more the "volume path"?

website/docs/r/netapp_volume.html.markdown

neil-yechenwei · 2019-12-11T08:06:20Z

Thank you for the comments @katbyte , I've updated code per your comments.

katbyte

Thanks for the revisions @neil-yechenwei,

In addition to the comments i've left inline some tests are failing:

------- Stdout: -------
=== RUN   TestAccAzureRMNetAppVolume_complete
=== PAUSE TestAccAzureRMNetAppVolume_complete
=== CONT  TestAccAzureRMNetAppVolume_complete
--- FAIL: TestAccAzureRMNetAppVolume_complete (899.77s)
    testing.go:569: Step 0 error: errors during apply:
        
        Error: Error waiting for creation of NetApp Volume "acctest-NetAppVolume-191211182103728355" (Resource Group "acctestRG-netapp-191211182103728355"): Code="InternalServerError" Message="Error calling V1CreateIPRange: {\"code\":null,\"message\":\"Error creating IP range - Specified IP range clashes with an existing IP range\"}\n"

Error: Error waiting for creation of NetApp Volume "acctest-NetAppVolume-191211182103680430" (Resource Group "acctestRG-netapp-191211182103680430"): Code="InternalServerError" Message="The requested FileSystemAllocation was not found."

azurerm/internal/services/netapp/validation_test.go

azurerm/data_source_netapp_volume.go

azurerm/resource_arm_netapp_volume.go

katbyte

@neil-yechenwei,

there are still some issues that need to be addressed and multiple tests are failing:

------- Stdout: -------
=== RUN   TestAccAzureRMNetAppVolume_updateSubnet
=== PAUSE TestAccAzureRMNetAppVolume_updateSubnet
=== CONT  TestAccAzureRMNetAppVolume_updateSubnet
--- FAIL: TestAccAzureRMNetAppVolume_updateSubnet (167.89s)
    testing.go:569: Step 0 error: errors during apply:
        
        Error: Error waiting for creation of NetApp Volume "acctest-NetAppVolume-191212172909800478" (Resource Group "acctestRG-netapp-191212172909800478"): Code="InternalServerError" Message="Error calling V1CreateIPRange: {\"code\":null,\"message\":\"Error creating IP range - Specified IP range clashes with an existing IP range\"}\n"

i suggest running all tests in parallel

azurerm/resource_arm_netapp_volume.go

website/docs/r/netapp_volume.html.markdown

…ei/terraform-provider-azurerm into patch_netapp_volume

neil-yechenwei · 2019-12-13T05:39:30Z

@katbyte , I already used the command you provided before for testing. Below is my test result whose test cases are all passed. I am using the test command make testacc TEST=./azurerm TESTARGS="-parallel 11 -test.run=TestAccAzureRMNetAppVolume". If it's different with yours, could you share your test command. So I think maybe it's just your test environment issue. Could you double check whether you deleted all NetApp related resources in Azure portal and make sure there is no your own changes on your local before testing? After you deleted all NetApp related resource, please rerun the test cases and all test cases should be passed. For your other comments, I've updated code.

--- PASS: TestAccAzureRMNetAppVolume_complete (571.17s)
--- PASS: TestAccAzureRMNetAppVolume_update (774.01s)
--- PASS: TestAccAzureRMNetAppVolume_basic (968.05s)
--- PASS: TestAccAzureRMNetAppVolume_updateExportPolicyRule (1141.21s)
--- PASS: TestAccAzureRMNetAppVolume_updateSubnet (1546.39s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 1561.007s

katbyte

Thanks for the revisions @neil-yechenwei, tests pass and this LGTM now!

ghost · 2020-01-08T10:35:37Z

This has been released in version 1.40.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.40.0"
}
# ... other configuration ...

ghost · 2020-03-28T15:34:57Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

Add New Resource & Data Source: azurerm_netapp_volume

5ca5ec5

ghost added size/XXL documentation labels Nov 20, 2019

tombuildsstuff suggested changes Nov 20, 2019

View reviewed changes

tombuildsstuff added new-data-source new-resource service/netapp waiting-response labels Nov 20, 2019

katbyte added this to the v1.38.0 milestone Nov 21, 2019

Update code by comments

dfb0aba

neil-yechenwei requested a review from tombuildsstuff November 21, 2019 14:56

ghost removed the waiting-response label Nov 21, 2019

katbyte requested changes Nov 22, 2019

View reviewed changes

katbyte added the waiting-response label Nov 22, 2019

v-cheye added 2 commits November 22, 2019 09:37

Update code

6af8355

Update code by comment

b4116ff

neil-yechenwei requested a review from katbyte November 22, 2019 07:46

ghost removed the waiting-response label Nov 22, 2019

update code by comments

b3c4324

katbyte requested changes Dec 3, 2019

View reviewed changes

katbyte added the waiting-response label Dec 3, 2019

tombuildsstuff modified the milestones: v1.38.0, v1.39.0 Dec 3, 2019

Update code by comments

2f39d29

neil-yechenwei requested a review from katbyte December 4, 2019 04:41

ghost removed the waiting-response label Dec 4, 2019

Merge remote-tracking branch 'upstream/master' into patch_netapp_volume

660ab1f

katbyte requested changes Dec 11, 2019

View reviewed changes

v-cheye added 2 commits December 11, 2019 14:37

Update code per comments

a6d0866

Update code

9f34d83

neil-yechenwei requested a review from katbyte December 11, 2019 08:05

katbyte requested changes Dec 12, 2019

View reviewed changes

azurerm/internal/services/netapp/validation_test.go Outdated Show resolved Hide resolved

azurerm/data_source_netapp_volume.go Outdated Show resolved Hide resolved

azurerm/resource_arm_netapp_volume.go Outdated Show resolved Hide resolved

Update code

4db7c5c

tombuildsstuff modified the milestones: v1.39.0, v1.40.0 Dec 12, 2019

katbyte requested changes Dec 12, 2019

View reviewed changes

azurerm/resource_arm_netapp_volume.go Outdated Show resolved Hide resolved

azurerm/resource_arm_netapp_volume.go Outdated Show resolved Hide resolved

website/docs/r/netapp_volume.html.markdown Outdated Show resolved Hide resolved

katbyte added the waiting-response label Dec 12, 2019

v-cheye added 2 commits December 13, 2019 12:48

Update code

4f9d1d8

Merge branch 'patch_netapp_volume' of https://github.com/neil-yechenw…

10b7251

…ei/terraform-provider-azurerm into patch_netapp_volume

neil-yechenwei requested a review from katbyte December 13, 2019 05:33

ghost removed the waiting-response label Dec 13, 2019

update code

3c3779b

katbyte approved these changes Dec 18, 2019

View reviewed changes

katbyte merged commit 2ebd225 into hashicorp:master Dec 18, 2019

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

Update CHANGELOG.md to include #4933

36e1569

ghost locked and limited conversation to collaborators Mar 28, 2020


		* `location` - The Azure Region where the NetApp Volume exists.

		* `creation_token` - A unique file path for the volume.


		* `rule_index` - (Required) The index number of the rule.

		* `allowed_clients` - (Required) Client ingress specification as list with IPv4 CIDRs, IPv4 host addresses.


		* `service_level` - (Required) The service level of the file system. Valid values include `Premium`, `Standard`, or `Ultra`.

		* `subnet_id` - (Required) The ID of a Subnet in which the NetApp Volume, which must have the delegation Microsoft.NetApp/volumes. Changing this forces a new resource to be created.


		* `creation_token` - (Required) A unique file path for the volume. Used when creating mount targets.

		* `service_level` - (Required) The service level of the file system. Valid values include `Premium`, `Standard`, or `Ultra`.


		* `account_name` - (Required) The name of the NetApp account in which the NetApp Pool should be created. Changing this forces a new resource to be created.

		* `creation_token` - (Required) A unique file path for the volume. Used when creating mount targets. Changing this forces a new resource to be created.

Add New Resource & Data Source: azurerm_netapp_volume #4933

Add New Resource & Data Source: azurerm_netapp_volume #4933

Conversation

neil-yechenwei commented Nov 20, 2019 • edited by tombuildsstuff Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Nov 20, 2019

neil-yechenwei commented Nov 21, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Nov 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Dec 4, 2019 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Dec 11, 2019

katbyte left a comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

neil-yechenwei commented Dec 13, 2019 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Jan 8, 2020

ghost commented Mar 28, 2020

neil-yechenwei commented Nov 20, 2019 •

edited by tombuildsstuff

Loading

neil-yechenwei Nov 21, 2019 •

edited

Loading

neil-yechenwei Nov 21, 2019 •

edited

Loading

neil-yechenwei commented Dec 4, 2019 •

edited

Loading

neil-yechenwei Dec 11, 2019 •

edited

Loading

neil-yechenwei commented Dec 13, 2019 •

edited

Loading