-
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
New resource: azurerm_hci_cluster
#9134
Conversation
azurerm_hyper_converged_cluster
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.
Hi @neil-yechenwei thanks for this PR and this mostly looks good aside from some minor comment, please have a look, thanks
azurerm/internal/services/azurestackhci/tests/hyper_converged_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/azurestackhci/validate/hyper_converged_cluster.go
Outdated
Show resolved
Hide resolved
@ArcturusZhang , thanks for your comments. I've updated code. |
azurerm_hyper_converged_cluster
azurerm_hci_cluster
@katbyte , thanks for your comments. I've updated code. Please have an another look. Thanks. |
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.
LGTM aside from a couple comments
azurerm/internal/services/azurestackhci/tests/hci_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/azurestackhci/tests/hci_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
@katbyte , thanks for your comments. I've updated code. Please have a look. Thanks. |
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.
LGTM 👍
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 this PR!
I've taken a look through and mostly LGTM! Whilst since this is a new RP, can we opt-in the codegen for the ID parser (and validator, together with their tests), and also opt-in the new test style?
azurerm/internal/services/azurestackhci/hci_cluster_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/azurestackhci/parse/hci_cluster_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/azurestackhci/tests/hci_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/azurestackhci/validate/hci_cluster.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/azurestackhci/hci_cluster_resource.go
Outdated
Show resolved
Hide resolved
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 this PR!
I have left some additional mostly minor comments that once addressed this should be good to merge 👍
azurerm/internal/services/azurestackhci/validate/cluster_name.go
Outdated
Show resolved
Hide resolved
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've taken a look through and left some comments inline - can we fix these up so that this resource makes matches the way users are likely to use it?
Thanks!
} | ||
|
||
if existing.ID != nil && *existing.ID != "" { | ||
return tf.ImportAsExistsError("azurerm_hci_cluster", *existing.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.
this should be using the Resource ID formatter's ID here: id.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.
updated
"tenant_id": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, |
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.
isn't this statistically likely to be the current Tenant ID, so shouldn't this be Optional?
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.
Optional (and Computed*) and ForceNew, actually - since this could be defaulted and overwritten
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.
updated
resource_group_name = azurerm_resource_group.test.name | ||
location = azurerm_resource_group.test.location | ||
client_id = data.azurerm_client_config.current.client_id | ||
tenant_id = data.azurerm_client_config.current.tenant_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.
rather than using the current service principal, since this is expecting an AAD application can we spin one up and use that instead?
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.
Sorry. For the other comments you mentioned, I've submitted PR. However, for this comment, seems I have no permission to create an AAD application via "azuread_application" after tested. So I haven't updated test code against this case. Per the description of aadTenantId
in rest api spec, I assume this is expecting AAD identity. My understanding is that the service principal is one kind of AAD identity. So I assume current test case already covered the feature.
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.
you'll need to grant permissions to use the Azure AD API - unfortunately this has to provision an AD Application as a part of the test, reusing the SP would cause issues
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.
OK. Updated test code and it works fine after tested.
// SupportedResources returns the supported Resources supported by this Service | ||
func (r Registration) SupportedResources() map[string]*schema.Resource { | ||
return map[string]*schema.Resource{ | ||
"azurerm_hci_cluster": resourceArmHCICluster(), |
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.
this name isn't clear this is for Azure Stack HCI directly, should this become azurerm_stack_hci_cluster
?
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.
updated
## Example Usage | ||
|
||
```hcl | ||
data "azurerm_client_config" "current" {} |
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.
this should likely be using the AAD Application Data Source to look up an existing application, rather than the service principal, which it's highly unlikely is being reused for AAD?
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.
Sorry. For the other comments you mentioned, I've submitted PR. However, for this comment, seems I have no permission to create an AAD application via "azuread_application" after tested. So I haven't updated test code against this case. Per the description of aadTenantId
in rest api spec, I assume this is expecting AAD identity. My understanding is that the service principal is one kind of AAD identity. So I assume current test case already covered the feature.
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.
OK. Updated test code and it works fine after tested.
|
||
tags = { | ||
ENV = "Prod" | ||
} |
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.
tags aren't a required field, can we remove these from the example?
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.
updated
|
||
* `tenant_id` - (Required) The Tenant ID of the Azure Active Directory which is used by the HCI Cluster. Changing this forces a new resource to be created. | ||
|
||
* `tags` - (Optional) A mapping of tags which should be assigned to the HCI Cluster. |
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.
for all of these, can we use Azure Stack HCI Cluster
rather than HCI Cluster
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.
updated
* `create` - (Defaults to 30 minutes) Used when creating the HCI Cluster. | ||
* `read` - (Defaults to 5 minutes) Used when retrieving the HCI Cluster. | ||
* `update` - (Defaults to 30 minutes) Used when updating the HCI Cluster. | ||
* `delete` - (Defaults to 30 minutes) Used when deleting the HCI Cluster. |
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.
(same here)
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.
Updated to Azure Stack HCI Cluster
|
||
The following attributes are exported: | ||
|
||
* `id` - The ID of the HCI Cluster. |
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.
(same here)
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.
Updated to Azure Stack HCI Cluster
This has been released in version 2.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 = "~> 2.40.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! |
Currently, terraform doesn't support to provision Azure Stack HCI Cluster resource. So I submit this PR to support it.
The overview of Azure Stack HCI: https://docs.microsoft.com/en-us/azure-stack/hci/
API spec: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2020-10-01/azurestackhci.json
--- PASS: TestAccHCICluster_complete (126.11s)
--- PASS: TestAccHCICluster_basic (126.12s)
--- PASS: TestAccHCICluster_update (168.28s)
--- PASS: TestAccHCICluster_requiresImport (202.93s)