-
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_data_protection_backup_vault
: support soft_delete
and retention_duration_in_days
properties
#24775
azurerm_data_protection_backup_vault
: support soft_delete
and retention_duration_in_days
properties
#24775
Conversation
…m-to-its-own-servicepackage refactor: splitting `ManagedHSM` out into it’s own Service Package
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 @sinbai! I am seeing test failures though. Do you mind taking a look at my comment and the tests?
Error: creating DataProtection BackupVault ("Backup Vault (Subscription: \"1a6092a6-137e-4025-9a7c-ef77f76f2c02\"\nResource Group Name: \"acctest-dataprotection-240205154228526486\"\nBackup Vault Name: \"acctest-bv-240205154228526486\")"): performing CreateOrUpdate: unexpected status 400 with error: BMSUserErrorInvalidInput: The User input provided for the call is invalid
with azurerm_data_protection_backup_vault.test,
on terraform_plugin_test.tf line 28, in resource "azurerm_data_protection_backup_vault" "test":
28: resource "azurerm_data_protection_backup_vault" "test" {
--- FAIL: TestAccDataProtectionBackupVault_basic (33.15s)
@@ -47,6 +47,10 @@ The following arguments are supported: | |||
|
|||
* `identity` - (Optional) An `identity` block as defined below. | |||
|
|||
* `soft_delete_setting` - (Optional) The state of soft delete for this Backup Vault. Possible values are `AlwaysOn`, `Off` and `On`. Defaults to `On`. | |||
|
|||
-> **Note:** Once the `soft_delete_setting` is set to `AlwaysOn`, the setting cannot be changed. |
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 add a customize diff that forces a new resource to be created when changing this value from true to 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.
Fixed.
azurerm_data_protection_backup_vault
: support soft_delete_setting
propertyazurerm_data_protection_backup_vault
: support soft_delete_setting
and retention_duration_in_days
properties
Hi @mbfrahry thanks for your feedback. I have fixed the above test failure and the comment. Could you please take another look? |
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 @sinbai, this was close but the way it is implemented will break break users when they update to this version of the provider. We shouldn't change the basic test as that is the bare minimum to get the resource up and running and changing it hid this breaking change.
state := d.Get("soft_delete_setting").(string) | ||
_, ok := d.GetOk("retention_duration_in_days") | ||
if state == string(backupvaults.SoftDeleteStateOn) && !ok { | ||
return fmt.Errorf("`retention_duration_in_days` is required when the `soft_delete_setting` is `On`") |
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.
The way this is implemented will break users as the default is on but retention_duration_in_days
hasn't been added to the provider yet. So when people update to this version of the provider, Terraform will error with this message. We should probably default retention_duration_in_days
or remove this check.
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 @mbfrahry thanks a lot. I have updated the code. Could you please take another look?
|
||
-> **Note:** The `retention_duration_in_days` is the number of days for which deleted data is retained before being permanently deleted. Retention period till 14 days are free of cost, however, retention beyond 14 days may incur additional charges. The `retention_duration_in_days` is required when the `soft_delete_setting` is set to `On`. | ||
|
||
* `soft_delete_setting` - (Optional) The state of soft delete for this Backup Vault. Possible values are `AlwaysOn`, `Off` and `On`. Defaults to `On`. |
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 think setting is redundant? as it is implied?
* `soft_delete_setting` - (Optional) The state of soft delete for this Backup Vault. Possible values are `AlwaysOn`, `Off` and `On`. Defaults to `On`. | |
* `soft_delete` - (Optional) The state of soft delete for this Backup Vault. Possible values are `AlwaysOn`, `Off` and `On`. Defaults to `On`. |
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 thanks for your feedback. The code has been updated. Could you please take another look?
Test results: https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_DATAPROTECTION/102824?buildTab=tests
azurerm_data_protection_backup_vault
: support soft_delete_setting
and retention_duration_in_days
propertiesazurerm_data_protection_backup_vault
: support soft_delete
and retention_duration_in_days
properties
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 🗑️
…etention_duration_in_days` properties (hashicorp#24775) * update code * update code * update code * update code --------- Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>changes detected:
	"hashicorp/azurerm" updated from "3.92.0" to "3.93.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.93.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.93.0
* **New Data Source**: `azurerm_express_route_circuit_peering` ([#24971](hashicorp/terraform-provider-azurerm#24971 **New Data Source**: `azurerm_storage_table_entities` ([#24973](hashicorp/terraform-provider-azurerm#24973 **New Resource**: `azurerm_dev_center_catalog` ([#24833](hashicorp/terraform-provider-azurerm#24833 **New Resource**: `azurerm_system_center_virtual_machine_manager_server` ([#24278](https://github.com/hashicorp/terraform-provider-azurerm/issues/24278))

BUG FIXES:

* `azurerm_key_vault` - conditionally polling the Data Plane endpoint when `public_network_access_enabled` is set to false ([#23823](hashicorp/terraform-provider-azurerm#23823 `azurerm_storage_account` - allow the `identity.type` property to be `SystemAssigned, UserAssigned` when using a Customer Managed Key ([#24923](hashicorp/terraform-provider-azurerm#24923 `azurerm_automation_account` - prevent the `identity.identity_ids` User Assigned identity being set when not specified in config ([#24977](https://github.com/hashicorp/terraform-provider-azurerm/issues/24977))

ENHANCEMENTS:

* dependencies: updating to `v0.20240221.1170458` of `hashicorp/go-azure-sdk` ([#24967](hashicorp/terraform-provider-azurerm#24967 dependencies: refactor `azurerm_spring_cloud_configuration_service` to use `go-azure-sdk` ([#24918](hashicorp/terraform-provider-azurerm#24918 provider: support or the feature flag `virtual_machine_scale_set.reimage_on_manual_upgrade` ([#22975](hashicorp/terraform-provider-azurerm#22975 `sentinel`: updating to use the transport layer from `hashicorp/go-azure-sdk` rather than `Azure/go-autorest` ([#24962](hashicorp/terraform-provider-azurerm#24962 `sqlvirtualmachines`: updating to use the transport layer from `hashicorp/go-azure-sdk` rather than `Azure/go-autorest` ([#24912](hashicorp/terraform-provider-azurerm#24912 `nginx` : updating to use `2024-01-01-preview` ([#24868](hashicorp/terraform-provider-azurerm#24868 `azurerm_cosmosdb_account` - support for the `backup.tier` property ([#24595](hashicorp/terraform-provider-azurerm#24595 `azurerm_linux_virtual_machine` - the `virtual_machine_scale_set_id` proeprty can now be changed without creating a new resource ([#24768](hashicorp/terraform-provider-azurerm#24768 `azurerm_machine_learning_workspace` - support for the `managed_network.isolation_mode` property ([#24951](hashicorp/terraform-provider-azurerm#24951 `azurerm_private_dns_resolver_inbound_endpoint` - support the `static` value for the `private_ip_allocation_method` property ([#24952](hashicorp/terraform-provider-azurerm#24952 `azurerm_postgresql_flexible_server` - expose the `storage_tier` field ([#24892](hashicorp/terraform-provider-azurerm#24892 `azurerm_redis_cache` - support for the `preferred_data_persistence_auth_method` property ([#24370](hashicorp/terraform-provider-azurerm#24370 `azurerm_servicebus_namespace` - support for the `premium_messaging_partitions` property ([#24676](hashicorp/terraform-provider-azurerm#24676 `azurerm_windows_virtual_machine` - the `virtual_machine_scale_set_id` proeprty can now be changed without creating a new resource ([#24768](https://github.com/hashicorp/terraform-provider-azurerm/issues/24768))

BUG FIXES:

* `azurerm_cognitive_deployment` - the `version_upgrade_option` property can not be updated without creating a new resource ([#24922](hashicorp/terraform-provider-azurerm#24922 `azurerm_data_protection_backup_vault` - support or the `soft_delete` and `retention_duration_in_days` properties ([#24775](hashicorp/terraform-provider-azurerm#24775 `azurerm_data_factory_pipeline` - correctly handle incorrect header values ([#24921](hashicorp/terraform-provider-azurerm#24921 `azurerm_kusto_cluster` - `optimized_auto_scale` is now updated after `sku` has been updated ([#24906](hashicorp/terraform-provider-azurerm#24906 `azurerm_key_vault_certificate` - will now only update the `lifetime_action` of the certificate block unless otherwise required ([#24755](hashicorp/terraform-provider-azurerm#24755 `azurerm_linux_virtual_machine_scale_set` - correctly include `public_ip_prefix_id` during updates ([#24939](hashicorp/terraform-provider-azurerm#24939 `azurerm_postgresql_flexible_server` - the `customer_managed_key.key_vault_key_id` property is now required ([#24981](hashicorp/terraform-provider-azurerm#24981 `azurerm_nginx_deployment` - changing the `sku` property now creates a new resource ([#24905](hashicorp/terraform-provider-azurerm#24905 `azurerm_orchestrated_virtual_machine_scale_set` - the `disk_size_gb` and `lun` parameters of `data_disks` are optional now ([#24944](hashicorp/terraform-provider-azurerm#24944 `azurerm_storage_account` - change order of API calls to be GET-then-PUT ratehr then PATCHES ([#23935](hashicorp/terraform-provider-azurerm#23935 `azurerm_storage_account` - improve the validation around the `immutability_policy` being used with `blob_properties` ([#24938](hashicorp/terraform-provider-azurerm#24938 `azurerm_security_center_setting` - prevent a bug when name is `SENTINEL` ([#24497](hashicorp/terraform-provider-azurerm#24497 `azurerm_windows_virtual_machine_scale_set` - correctly include `public_ip_prefix_id` during updates ([#24939](https://github.com/hashicorp/terraform-provider-azurerm/issues/24939))




</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/19/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
…etention_duration_in_days` properties (hashicorp#24775) * update code * update code * update code * update code --------- Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
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. |
Swagger : https://github.com/Azure/azure-rest-api-specs/blob/79e4e0c9f8e1c134bcda069fc7994fd99599ccdc/specification/dataprotection/resource-manager/Microsoft.DataProtection/stable/2023-05-01/dataprotection.json#L6970
Test results: