-
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
azurerm_key_vault_managed_hardware_security_module
: remove ForceNew
for public_network_access_enabled
#26075
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.
Thanks @wuxu92. This looks mostly fine, but we need to use a different test to validate the update of this property. The basic test configs should only contain required properties.
tenant_id = data.azurerm_client_config.current.tenant_id | ||
admin_object_ids = [data.azurerm_client_config.current.object_id] | ||
purge_protection_enabled = false | ||
public_network_access_enabled = 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.
This is the config for the basic test which shouldn't have Optional
properties in it. Can you please find another config/test to use to validate this case?
public_network_access_enabled = 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.
Sure, the public_network_access_enabled
property is actually set to True by default. Therefore, I can simply remove this line from the basic config. However, can we still use basicUpdate
to test the update of this property? Since most of the required properties of MSHM are also ForceNew, the basicUpdate
config actually updates other non-required properties such as tags
and network_acls
.
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.
Sure, that's acceptable!
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.
removed from basic.
tenant_id = data.azurerm_client_config.current.tenant_id | ||
admin_object_ids = [data.azurerm_client_config.current.object_id] | ||
purge_protection_enabled = false | ||
public_network_access_enabled = false |
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, from the name of the config this is only checking whether the required properties are updateable.
public_network_access_enabled = false |
Thanks @wuxu92, the Terraform Schema Linting check just needs to be addressed now. |
Thanks @stephybun linting issue addressed. |
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 @wuxu92 LGTM 👍
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. |
Community Note
Description
The
public_network_access_enabled
should support update for manged HSM.PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_key_vault_managed_hardware_security_module
- support updatepublic_network_access_enabled
propertyThis is a (please select all that apply):
Related Issue(s)
Fixes #26044
Note
If this PR changes meaningfully during the course of review please update the title and description as required.