-
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_data_protection_backup_policy_kubernetes_cluster
#24718
New resource: azurerm_data_protection_backup_policy_kubernetes_cluster
#24718
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 @sinbai, Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍
ValidateFunc: validation.StringInSlice([]string{ | ||
string(backuppolicies.AbsoluteMarkerAllBackup), | ||
string(backuppolicies.AbsoluteMarkerFirstOfDay), | ||
string(backuppolicies.AbsoluteMarkerFirstOfMonth), | ||
string(backuppolicies.AbsoluteMarkerFirstOfWeek), | ||
string(backuppolicies.AbsoluteMarkerFirstOfYear), | ||
}, 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.
shall we use validation.StringInSlice(backuppolicies.PossibleValuesFor...(), false),
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.
Fixed.
ValidateFunc: validation.StringInSlice([]string{ | ||
string(backuppolicies.WeekNumberFirst), | ||
string(backuppolicies.WeekNumberSecond), | ||
string(backuppolicies.WeekNumberThird), | ||
string(backuppolicies.WeekNumberFourth), | ||
string(backuppolicies.WeekNumberLast), | ||
}, 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.
shall we use validation.StringInSlice(backuppolicies.PossibleValuesFor...(), false),
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.
Fixed.
ValidateFunc: validate.ISO8601Duration, | ||
}, | ||
|
||
"target_copy_setting": { |
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.
default_retention_rule.target_copy_setting
is not found in testcase?
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 target_copy_setting
property has been removed.
model := resp.Model | ||
if model == nil { | ||
return fmt.Errorf("retrieving %s: model was nil", 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.
please use if model := resp.Model; model != nil {....
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.
resp, err := client.Get(ctx, *id) | ||
if err != nil { | ||
if response.WasNotFound(resp.HttpResponse) { | ||
return metadata.MarkAsGone(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.
return metadata.MarkAsGone(id) | |
return metadata.MarkAsGone(*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.
Fixed.
}) | ||
} | ||
|
||
func TestAccDataProtectionBackupPolicyKubernatesCluster_update(t *testing.T) { |
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 all properties are forceNew
, this update
can be removed?
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.
Yes, fixed.
|
||
A `life_cycle` block supports the following: | ||
|
||
* `data_store_type` - (Required) The type of data store. Possible values is `OperationalStore`. |
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.
* `data_store_type` - (Required) The type of data store. Possible values is `OperationalStore`. | |
* `data_store_type` - (Required) The type of data store. The only possible value is `OperationalStore`. |
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.
|
||
* `copy_after` - (Required) Specifies when the backups are tiered across two or more selected data stores as a json encoded string. Changing this forces a new resource to be created. | ||
|
||
* `data_store_type` - (Required) The target copy data store type. Possible values is `OperationalStore`. |
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.
* `data_store_type` - (Required) The target copy data store type. Possible values is `OperationalStore`. | |
* `data_store_type` - (Required) The target copy data store type. The only possible value is `OperationalStore`. |
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.
life_cycle { | ||
duration = "P7D" | ||
data_store_type = "OperationalStore" | ||
target_copy_setting { | ||
copy_after = jsonencode({ | ||
objectType = "ImmediateCopyOption" | ||
}) | ||
data_store_type = "OperationalStore" | ||
} | ||
} | ||
|
||
life_cycle { | ||
duration = "P84D" | ||
data_store_type = "OperationalStore" | ||
} |
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.
does it make sense to have 2 life_cycle
blocks with the same data_store_type
?
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.
|
||
A `target_copy_setting` block supports the following: | ||
|
||
* `copy_after` - (Required) Specifies when the backups are tiered across two or more selected data stores as a json encoded string. 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.
since there is only one type of data_store_type
. Do there exist a real scenario that the backups are tiered across two or more selected data stores
?
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.
Thank you for your question. Yes, this has been confirmed with the services team, but is currently in preview (feature is already supported in Azure Portal). Considering that the service team did not request onboard this preview feature this time, I deleted target_copy_setting
in the code, set to empty array.
Hi @ms-zhenhua thanks for your time and valuable feedback. I have fixed/commented feedback. Could you please take another look? Retested results are as follows: |
results := backuppolicies.AzureRetentionRule{} | ||
if len(input) == 0 { | ||
return results | ||
} |
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.
if len(input)==0
, we should not return an empty rule. In this case, no rule should be added to the policyRules
list outside this method?
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 your feedback, fixed.
c9b8495
to
3c757cd
Compare
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 📏
<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.90.0" to "3.91.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.91.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.91.0
FEATURES:

* **New Data Source**: `azurerm_databricks_access_connector` ([#24769](hashicorp/terraform-provider-azurerm#24769 **New Resource**: `azurerm_data_protection_backup_policy_kubernetes_cluster` ([#24718](hashicorp/terraform-provider-azurerm#24718 **New Resource**: `azurerm_chaos_studio_experiment` ([#24779](hashicorp/terraform-provider-azurerm#24779 **New Resource**: `azurerm_chaos_studio_capability` ([#24779](hashicorp/terraform-provider-azurerm#24779 **New Resource**: `azurerm_dev_center_gallery` ([#23760](hashicorp/terraform-provider-azurerm#23760 **New Resource:** `azurerm_kubernetes_fleet_member` ([#24792](hashicorp/terraform-provider-azurerm#24792 **New Resource:** `azurerm_iotcentral_organization` ([#23132](hashicorp/terraform-provider-azurerm#23132 **New Resource:** `azurerm_spring_cloud_app_dynamics_application_performance_monitoring` ([#24750](https://github.com/hashicorp/terraform-provider-azurerm/issues/24750))

ENHANCEMENTS:

* dependencies: updating to `v0.20240208.1095436` of `github.com/hashicorp/go-azure-sdk/resource-manager` ([#24819](hashicorp/terraform-provider-azurerm#24819 dependencies: updating to `v0.20240208.1095436` of `github.com/hashicorp/go-azure-sdk/sdk` ([#24819](hashicorp/terraform-provider-azurerm#24819 dependencies: refactor `azurerm_app_service_environment_v3` to use `go-azure-sdk` ([#24760](hashicorp/terraform-provider-azurerm#24760 dependencies: refactor `azurerm_role_definition` to use `go-azure-sdk` ([#24266](hashicorp/terraform-provider-azurerm#24266 `managedhsm`: updating to use the transport layer from `hashicorp/go-azure-sdk` rather than `Azure/go-autorest` ([#24761](hashicorp/terraform-provider-azurerm#24761 `hdinsight`: updating to API Version `2023-07-01` ([#24761](hashicorp/terraform-provider-azurerm#24761 `streamanalytics`: updating to use the transport layer from `hashicorp/go-azure-sdk` rather than `Azure/go-autorest` ([#24819](hashicorp/terraform-provider-azurerm#24819 `azurerm_app_service_environment_v3` - support for the `remote_debugging_enabled` property ([#24760](hashicorp/terraform-provider-azurerm#24760 `azurerm_storage_account` - support for the `local_user_enabled` property ([#24800](hashicorp/terraform-provider-azurerm#24800 `azurerm_log_analytics_workspace_table` - support for the `total_retention_in_days` property ([#24513](hashicorp/terraform-provider-azurerm#24513 `azurerm_maching_learning_workspace` - support for the `feature_store` and `kind` properties ([#24716](hashicorp/terraform-provider-azurerm#24716 `azurerm_traffic_manager_azure_endpoint` - support for the `always_serve_enabled` property ([#24573](hashicorp/terraform-provider-azurerm#24573 `azurerm_traffic_manager_external_endpoint` - support for the `always_serve_enabled` property ([#24573](https://github.com/hashicorp/terraform-provider-azurerm/issues/24573))

BUG FIXES:

* `azurerm_api_management` - the `virtual_network_configuration` property now updates correctly outside of `virtual_network_type` ([#24569](https://github.com/hashicorp/terraform-provider-azurerm/issues/24569))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/1083/">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>
…er` (hashicorp#24718) * resolve conflict * update code * update code to fix internal review feedback * update code * update code
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/f3cd6922dbe117d78b4f719bbf8b03db46b30808/specification/dataprotection/resource-manager/Microsoft.DataProtection/stable/2023-05-01/dataprotection.json#L864
Test results: