-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New Resource: azurerm_hpc_cache
#5528
Conversation
…, tag formmating, docs updates
azurerm_hpc_cache
azurerm_hpc_cache
PR should be ready for review now. One note is I didn't add support for tags b/c I was running into a strange issue where |
Tests Pass:
|
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.
hey @aqche
Thanks for this PR - apologies for the delayed review here!
I've taken a look through and left some comments inline, but this is mostly looking good to me - I'll send a request to opt us into the HPC Cache RP now too 👍
Thanks!
HPC Cache can be imported using the `resource id`, e.g. | ||
|
||
```shell | ||
terraform import azurerm_hpc_cache.example /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/resourceGroupName/providers/Microsoft.StorageCache/caches/cacheName |
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.
since this is a new Resource Provider (Microsoft.StorageCache
) could we add that to this list
website/azurerm.erb
Outdated
@@ -2224,6 +2224,15 @@ | |||
</ul> | |||
</li> | |||
|
|||
<li> | |||
<a href="#">Storage Cache Resources</a> |
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.
rather than making this it's own group could we move this into the Storage list?
// SupportedResources returns the supported Resources supported by this Service | ||
func (r Registration) SupportedResources() map[string]*schema.Resource { | ||
return map[string]*schema.Resource{ | ||
"azurerm_hpc_cache": resourceArmHPCCache(), |
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.
rather than making this it's own service could we move this into the Storage service, since it forms part of that?
|
||
return &Client{ | ||
CachesClient: &cachesClient, | ||
} |
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.
rather than making this it's own service could we move this into the Storage service, since it forms part of that?
Looks like website-lint is failing to something unrelated to these PR changes. edit: fixed after merging master 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.
@aqche , Thanks for the PR and pushing the updates... This LGTM now! 🚀
return fmt.Errorf("Error creating HPC Cache %q (Resource Group %q): %+v", name, resourceGroup, err) | ||
} | ||
|
||
stateConf := &resource.StateChangeConf{ |
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.
@aqche
I may be dumb here, why not just use future.WaitForCompletionRef()
here? Is there some limitation about this API?
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.
@magodo thanks for calling that out, it actually slipped my mind to try future.WaitForCompletionRef()
here first. I'll give it a test and report back.
update: worked like a charm!
@tombuildsstuff @WodansSon made a small update to use
|
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 revisions @aqche, have left some additional mostly minor comments that once addressed this should be good to merge
id, err := azure.ParseAzureResourceID(d.Id()) | ||
if err != nil { | ||
return err | ||
} | ||
resourceGroup := id.ResourceGroup | ||
name := id.Path["caches"] |
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 are starting to write Parse ID functions for all resources, you can see an example here: #5779
azurerm/internal/services/storage/tests/resource_arm_hpc_cache_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/storage/tests/resource_arm_hpc_cache_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/storage/tests/resource_arm_hpc_cache_test.go
Outdated
Show resolved
Hide resolved
|
||
Manages a HPC Cache. | ||
|
||
~> **Note**: During the first several months of the GA release, a request must be made to the Azure HPC Cache team to add your subscription to the access list before it can be used to create a cache instance. |
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.
Could we add a link to where a user could do that?
location = "${azurerm_resource_group.example.location}" | ||
resource_group_name = "${azurerm_resource_group.example.name}" |
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.
Could we use .12 syntax for the docs?
Co-Authored-By: kt <[email protected]>
@katbyte, thanks for the review! make the updates you suggested 👍 Resource Test:
Parser Test:
|
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 revisions @aqche, LGTM aside from one missed comment, as per toms request we need to add this to the list of required resource providers. once that is done this is good to merge
@@ -62,6 +62,7 @@ func RequiredResourceProviders() map[string]struct{} { | |||
"Microsoft.ServiceFabric": {}, | |||
"Microsoft.Sql": {}, | |||
"Microsoft.Storage": {}, | |||
"Microsoft.StorageCache": {}, |
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.
@katbyte i've updated the RequiredResourceProviders
func here. let me know if this is correct. thanks!
@katbyte thanks for reviewing the PR again! added a review comment addressing your last request. (just commenting so the |
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 @aqche! LGTM, now just waiting on approval from the HPC team so we can test this.
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 👉 [email protected]. Thanks! |
Fixes: #5504
Adds the
azurerm_hpc_cache
resource.