-
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 and data source "azurerm_key_vault_managed_hardware_security_module" #10873
Conversation
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 @njuCZ
Thanks for this PR - I've taken a look through and left some comments inline, but if we can fix those up then we should be able to take another look through and run the tests.
Thanks!
azurerm/internal/services/keyvault/key_vault_managed_hardware_security_module_data_source..go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/keyvault/key_vault_managed_hardware_security_module_data_source..go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/keyvault/key_vault_managed_hardware_security_module_data_source..go
Show resolved
Hide resolved
azurerm/internal/services/keyvault/key_vault_managed_hardware_security_module_resource..go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/keyvault/key_vault_managed_hardware_security_module_resource..go
Outdated
Show resolved
Hide resolved
website/docs/d/key_vault_managed_hardware_security_module.html.markdown
Outdated
Show resolved
Hide resolved
|
||
* `purge_protection_enabled` - (Optional) Is Purge Protection enabled for this Key Vault Managed Hardware Security Module? Defaults to `false`. Changing this forces a new resource to be created. | ||
|
||
* `soft_delete_retention_days` - (Optional) The number of days that items should be retained for once soft-deleted. This value can be between `7` and `90` days. Defaults to `90`. Changing this forces a new resource to be created. |
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 isn't defaulted in the schema - and from memory Terraform intentionally defaults to 7
for the other resource, so we should match that 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.
for the key vault resource, it's also default 90 https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/services/keyvault/key_vault_resource.go#L196
website/docs/r/key_vault_managed_hardware_security_module.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/key_vault_managed_hardware_security_module.html.markdown
Outdated
Show resolved
Hide resolved
it seems the CI error is because of temporary network break |
rebase and run the acctest in sequence |
azurerm/internal/services/keyvault/key_vault_managed_hardware_security_module_resource..go
Show resolved
Hide resolved
thanks for the commit |
@katbyte I have made the acctest run in sequence. Every test has used the func |
ed1fe9e
to
3129f77
Compare
@katbyte thanks for the suggestions. I have changed the way to run test case. Now all acctest have passed in teamcity |
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 @njuCZ - LGTM and once the merge conflict is resolved we can get this merged
…security_module_resource..go
@katbyte Have rebased and fix the conflicts |
This has been released in version 2.57.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.57.0"
}
# ... other configuration ... |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
first pr to fix issue: #10209
3.1
soft_delete
is alwaystrue
. If setting tofalse
in the create rest api, it will report error3.2
soft_delete_retention_days
could not be updated, the backend service will report error3.3
purge_protection_enabled
could be set in the update rest api, but the response remain the same. this is an issue and I have reported to the service team3.4 I have made the above fields
forcenew