-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_netapp_*
- support for tags
& add ID parsing functions
#5995
Conversation
ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) | ||
defer cancel() | ||
|
||
id, err := azure.ParseAzureResourceID(d.Id()) |
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 realize the team is creating parsers for resource ids, would you like me to go ahead and create parsers for each of the netapp resoures or let that be addressed in another PR?
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.
It would be preferred to do it now but either or is fine 🙂
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.
sounds good, i'll get parsers added in
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.
Thanks for the PR @aqche, overall this looks good and aside from some minor comments about style the only thing i'm concerned about is capitals in the tags so we can confirm the api handles case correctly.
azurerm/internal/services/netapp/tests/resource_arm_netapp_volume_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/netapp/tests/resource_arm_netapp_volume_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/netapp/tests/resource_arm_netapp_snapshot_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/netapp/tests/resource_arm_netapp_snapshot_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/netapp/tests/resource_arm_netapp_pool_test.go
Outdated
Show resolved
Hide resolved
_, err = client.Update(ctx, parameters, resourceGroup, accountName, poolName, volumeName, name) | ||
if err != nil { |
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.
these two lines can be merged
if resp.ID == nil || *resp.ID == "" { | ||
return fmt.Errorf("Cannot read NetApp Snapshot %q (Resource Group %q) ID", name, resourceGroup) | ||
} | ||
d.SetId(*resp.ID) |
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.
we should only be setting ID on create
ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) | ||
defer cancel() | ||
|
||
id, err := azure.ParseAzureResourceID(d.Id()) |
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.
It would be preferred to do it now but either or is fine 🙂
azurerm/internal/services/netapp/resource_arm_netapp_snapshot.go
Outdated
Show resolved
Hide resolved
Co-Authored-By: kt <kt@katbyte.me>
azurerm_netapp_*
- support for tags
azurerm_netapp_*
- support for tags
& add ID parsing functions
@katbyte thanks for the review! made the updates you suggested and added the parse functions. I still don't have access to NetApp but here are the parse function test results:
|
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.
Thanks for going above and beyond in adding those functions @aqche, LGTM 👍
This has been released in version 2.1.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.1.0"
}
# ... other configuration ... |
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! |
Fixes: #5532
Adds the
tags
option for NetApp resources.I wrote the code, but I didn't realize you need to request access to NetApp before you can create resources, so I an unable to run the tests atm. I'll put in a request, but if someone has time to run the tests that would be helpful as well!