azurerm_netapp_volume - support mount_ip_addresses #5526

neil-yechenwei · 2020-01-26T12:33:38Z

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments inline but the main question I've got is regarding the resource design, if we can clarify that then we should be able to determine how to proceed here.

Thanks!

tombuildsstuff · 2020-02-04T10:16:03Z

azurerm/internal/services/netapp/resource_arm_netapp_volume.go

+	for _, item := range input.([]interface{}) {
+		if item != nil {
+			v := item.(map[string]interface{})
+			ipAddress := v["ipAddress"]


this needs to be:

Suggested change

ipAddress := v["ipAddress"]

ipAddress := v["ip_address"].(string)

It cannot be updated to "ip_address" since api returns ""mountTargets":[{"provisioningState":"Succeeded","ipAddress":"10.0.2.4"}]" for property mountTargets.

ah so this nested object isn't modelled as an object in the Swagger? in which case can we send a PR to the Swagger repo to fix that

When it comes to mountTargets, it reminds me that I changed that some time ago... and it may be a mistake. I will take a look at this and see how to change it correctly (and when changing the swagger we should let the service team know and approve, which may take a long time).

tombuildsstuff · 2020-02-04T10:16:55Z

azurerm/internal/services/netapp/data_source_netapp_volume.go

@@ -42,6 +42,19 @@ func dataSourceArmNetAppVolume() *schema.Resource {
 				ValidateFunc: ValidateNetAppPoolName,
 			},

+			"mount_targets": {
+				Type:     schema.TypeSet,


this needs to be a TypeList if it's a Computed field else it's not easily indexable - however - what are the mount targets here, a list of IP Addresses which should be used to mount the volume? If so it's likely this'd make more sense as a list of strings being mount_ip_addresses?

can we also update the test to confirm this field is correctly set

neil-yechenwei · 2020-02-05T03:37:27Z

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

tombuildsstuff

hey @neil-yechenwei

Thanks for making those changes - taking a look through whilst this looks good it appears we also need to send a PR to fix the Swagger prior to merging this, else breaking changes to the API Response won't be caught via the ARM "breaking changes" process - would you be able to send a PR to fix that in the Swagger and link that in the code here so that we can switch it out once fixed?

Thanks!

tombuildsstuff · 2020-02-11T09:16:59Z

website/docs/d/netapp_volume.html.markdown

@@ -43,6 +43,8 @@ The following attributes are exported:

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

+* `mount_ip_addresses` - A list of IPv4 Addresses which should be used to mount the volume.


this has also been added to the Resource - so can we document this field there too?

I didn't add this field "mount_ip_addresses" to the resource file. I just added this to data source file. So I think we don't need to document this field to resource md file.

Is there a reason we are only adding it to the datasource and not the resource too? @neil-yechenwei

You can find user's usage preference from (#5416), seems they want to use data source to retrieve this property value.

neil-yechenwei · 2020-02-11T11:00:45Z

hey @neil-yechenwei

Thanks for making those changes - taking a look through whilst this looks good it appears we also need to send a PR to fix the Swagger prior to merging this, else breaking changes to the API Response won't be caught via the ARM "breaking changes" process - would you be able to send a PR to fix that in the Swagger and link that in the code here so that we can switch it out once fixed?

Thanks!

@tombuildsstuff , I am not sure what you mean. Do you mean I need to make a fix to set property type of MountTargets as exact data type not interface{} like ExportPolicy in the Swagger, right? If not, may I know what & why I need to fix in the swagger?

katbyte

@neil-yechenwei,

Yes that is correct, what tom is referring to is that the MountTargets property is an interface without type

// MountTargets - List of mount targets
	MountTargets interface{} `json:"mountTargets,omitempty"`

when it should be a fully typed array

// MountTargets - List of mount targets
	MountTargets *[]VolumePropertiesMountTarget `json:"mountTargets,omitempty"`

neil-yechenwei · 2020-03-05T09:32:18Z

@katbyte , If you point to this issue, I've confirmed with service owner. They will fix this issue in the future but no ETA. So I've added a comment in the code and filed an issue (Azure/azure-rest-api-specs#8604) on rest api spec. Once they fix this issue, I will make an another PR to update code. So could you help to merge this PR first?

neil-yechenwei · 2020-05-18T07:21:54Z

@katbyte & @tombuildsstuff , I've applied the fix for property mountTargets after new go sdk is upgraded. Thanks.

katbyte · 2020-05-20T01:07:57Z

@neil-yechenwei - could we add the property to the resource to? thanks!

neil-yechenwei · 2020-05-20T05:07:15Z

@katbyte , I also added it to resource. Thanks.

katbyte

Thanks @neil-yechenwei - LGTM 👍

ghost · 2020-05-22T01:04:12Z

This has been released in version 2.11.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 = "~> 2.11.0"
}
# ... other configuration ...

ghost · 2020-06-20T14:50:06Z

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 more support for mountTargets

f810b05

ghost added size/S documentation labels Jan 26, 2020

v-cheye added 2 commits January 26, 2020 20:44

Update code

f607b5f

Update code

b79415e

neil-yechenwei requested a review from katbyte February 4, 2020 02:46

v-cheye added 2 commits February 4, 2020 11:00

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

eaaf155

Update code

c27bf6c

tombuildsstuff removed the request for review from katbyte February 4, 2020 10:14

tombuildsstuff suggested changes Feb 4, 2020

View reviewed changes

tombuildsstuff added enhancement service/netapp waiting-response labels Feb 4, 2020

tombuildsstuff added this to the v1.44.0 milestone Feb 4, 2020

Update code

3e43f53

ghost removed the waiting-response label Feb 5, 2020

neil-yechenwei requested a review from tombuildsstuff February 5, 2020 03:37

tombuildsstuff suggested changes Feb 11, 2020

View reviewed changes

tombuildsstuff added the sdk/requires-swagger-changes Changes need to be made in the Swagger specifications to enable this functionality label Feb 11, 2020

tombuildsstuff modified the milestones: v1.44.0, Blocked Feb 11, 2020

neil-yechenwei requested a review from tombuildsstuff February 11, 2020 11:00

katbyte requested changes Mar 5, 2020

View reviewed changes

katbyte added the waiting-response label Mar 5, 2020

Update code

566602b

ghost removed the waiting-response label Mar 5, 2020

neil-yechenwei requested a review from katbyte March 5, 2020 09:32

Update code

367f3ce

katbyte added sdk/requires-newer-api-version This requires upgrading the version of the API being used sdk/requires-upgrade This is dependent upon upgrading an SDK and removed waiting-response labels Apr 13, 2020

katbyte modified the milestones: v2.6.0, Blocked Apr 15, 2020

neil-yechenwei added 3 commits May 18, 2020 14:15

Update code

84e02a9

go mod vendor

70f7012

Update code

81f1171

ghost added size/XXL and removed size/XL labels May 18, 2020

neil-yechenwei added 2 commits May 18, 2020 14:56

Update code

1ca3640

Update code

10c4129

ghost added size/S and removed size/XXL labels May 18, 2020

katbyte removed sdk/requires-newer-api-version This requires upgrading the version of the API being used sdk/requires-upgrade This is dependent upon upgrading an SDK labels May 20, 2020

katbyte removed this from the Blocked milestone May 20, 2020

Update code

db409de

katbyte added this to the v2.11.0 milestone May 20, 2020

katbyte approved these changes May 20, 2020

View reviewed changes

katbyte changed the title ~~Add more support for mountTargets for azurerm_netapp_volume~~ azurerm_netapp_volume - support mount_ip_addresses May 20, 2020

katbyte merged commit b0501f9 into hashicorp:master May 20, 2020

katbyte added a commit that referenced this pull request May 20, 2020

update cHANGELOG.md to include #5526

8950850

ghost locked and limited conversation to collaborators Jun 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_netapp_volume - support mount_ip_addresses #5526

azurerm_netapp_volume - support mount_ip_addresses #5526

neil-yechenwei commented Jan 26, 2020 •

edited by tombuildsstuff

Loading

tombuildsstuff left a comment

tombuildsstuff Feb 4, 2020

neil-yechenwei Feb 5, 2020

tombuildsstuff Feb 11, 2020

ArcturusZhang Mar 23, 2020

tombuildsstuff Feb 4, 2020

neil-yechenwei Feb 5, 2020

neil-yechenwei commented Feb 5, 2020

tombuildsstuff left a comment

tombuildsstuff Feb 11, 2020

neil-yechenwei Feb 11, 2020

katbyte Mar 5, 2020

neil-yechenwei Mar 6, 2020 •

edited

Loading

neil-yechenwei commented Feb 11, 2020 •

edited

Loading

katbyte left a comment •

edited

Loading

neil-yechenwei commented Mar 5, 2020

neil-yechenwei commented May 18, 2020

katbyte commented May 20, 2020

neil-yechenwei commented May 20, 2020

katbyte left a comment

ghost commented May 22, 2020

ghost commented Jun 20, 2020

	ipAddress := v["ipAddress"]
	ipAddress := v["ip_address"].(string)

		@@ -43,6 +43,8 @@ The following attributes are exported:

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

		* `mount_ip_addresses` - A list of IPv4 Addresses which should be used to mount the volume.

azurerm_netapp_volume - support mount_ip_addresses #5526

azurerm_netapp_volume - support mount_ip_addresses #5526

Conversation

neil-yechenwei commented Jan 26, 2020 • edited by tombuildsstuff Loading

tombuildsstuff 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

neil-yechenwei commented Feb 5, 2020

tombuildsstuff 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

neil-yechenwei Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

neil-yechenwei commented Feb 11, 2020 • edited Loading

katbyte left a comment • edited Loading

Choose a reason for hiding this comment

neil-yechenwei commented Mar 5, 2020

neil-yechenwei commented May 18, 2020

katbyte commented May 20, 2020

neil-yechenwei commented May 20, 2020

katbyte left a comment

Choose a reason for hiding this comment

ghost commented May 22, 2020

ghost commented Jun 20, 2020

neil-yechenwei commented Jan 26, 2020 •

edited by tombuildsstuff

Loading

neil-yechenwei Mar 6, 2020 •

edited

Loading

neil-yechenwei commented Feb 11, 2020 •

edited

Loading

katbyte left a comment •

edited

Loading