`azurerm_mssql_managed_database` - support for retention policies #20845

catriona-m · 2023-03-08T14:10:00Z

No description provided.

katbyte

Aside from 2 comments LGTM 💾

katbyte · 2023-03-08T17:19:31Z

website/docs/r/mssql_managed_database.html.markdown

@@ -61,8 +61,21 @@ The following arguments are supported:

 * `managed_instance_id` - (Required) The ID of the Azure SQL Managed Instance on which to create this Managed Database. Changing this forces a new resource to be created.

+* `long_term_retention_policy` - (Optional) A `long_term_retention_policy` block as defined below.
+
+* `short_term_retention_days` - (Optional) The backup retention period in days. This is how many days Point-in-Time Restore will be supported.


Suggested change

* `short_term_retention_days` - (Optional) The backup retention period in days. This is how many days Point-in-Time Restore will be supported.

* `short_term_retention_days` - (Optional) The backup retention period in days. This is how many days Point-in-Time Restore will be supported for.

katbyte · 2023-03-08T17:20:18Z

website/docs/r/mssql_managed_database.html.markdown

+* `weekly_retention` - (Optional) The weekly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 520 weeks. e.g. `P1Y`, `P1M`, `P1W` or `P7D`.
+* `monthly_retention` - (Optional) The monthly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 120 months. e.g. `P1Y`, `P1M`, `P4W` or `P30D`.
+* `yearly_retention` - (Optional) The yearly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 10 years. e.g. `P1Y`, `P12M`, `P52W` or `P365D`.


should these be

Suggested change

* `weekly_retention` - (Optional) The weekly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 520 weeks. e.g. `P1Y`, `P1M`, `P1W` or `P7D`.

* `monthly_retention` - (Optional) The monthly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 120 months. e.g. `P1Y`, `P1M`, `P4W` or `P30D`.

* `yearly_retention` - (Optional) The yearly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 10 years. e.g. `P1Y`, `P12M`, `P52W` or `P365D`.

* `weekly_retention_duration` - (Optional) The weekly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 520 weeks. e.g. `P1Y`, `P1M`, `P1W` or `P7D`.

* `monthly_retention_duration` - (Optional) The monthly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 120 months. e.g. `P1Y`, `P1M`, `P4W` or `P30D`.

* `yearly_retention_duration` - (Optional) The yearly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 10 years. e.g. `P1Y`, `P12M`, `P52W` or `P365D`.

or even this as retention is in the block name?

Suggested change

* `weekly_retention` - (Optional) The weekly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 520 weeks. e.g. `P1Y`, `P1M`, `P1W` or `P7D`.

* `monthly_retention` - (Optional) The monthly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 120 months. e.g. `P1Y`, `P1M`, `P4W` or `P30D`.

* `yearly_retention` - (Optional) The yearly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 10 years. e.g. `P1Y`, `P12M`, `P52W` or `P365D`.

* `weekly_duration` - (Optional) The weekly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 520 weeks. e.g. `P1Y`, `P1M`, `P1W` or `P7D`.

* `monthly_duration` - (Optional) The monthly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 120 months. e.g. `P1Y`, `P1M`, `P4W` or `P30D`.

* `yearly_duration` - (Optional) The yearly retention policy for an LTR backup in an ISO 8601 format. Valid value is between 1 to 10 years. e.g. `P1Y`, `P12M`, `P52W` or `P365D`.

I've named them this way to keep them consistent with the equivalent in azurerm_mssql_database:

terraform-provider-azurerm/internal/services/mssql/helper/sql_retention_policies.go

Line 24 in fb309bb

"weekly_retention": {

But happy to change the names here if that makes more sense?

mbfrahry

Looks like there is a default value that is messing up the basic test as well

=== CONT  TestAccMsSqlManagedDatabase_basic
testcase.go:110: Step 1/2 error: After applying this test step, the plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# azurerm_mssql_managed_database.test will be updated in-place
~ resource "azurerm_mssql_managed_database" "test" {
id                        = "/subscriptions/*******/resourceGroups/acctestRG1-sql-230308173255406144/providers/Microsoft.Sql/managedInstances/acctestsqlserver230308173255406144/databases/acctest-230308173255406144"
name                      = "acctest-230308173255406144"
- short_term_retention_days = 7 -> null
# (1 unchanged attribute hidden)
# (1 unchanged block hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.

jackofallops

Hi @catriona-m - Spotted a couple things while passing through, I think the tests have highlighted one of the points on the default value for short_term_retention_days?

jackofallops · 2023-03-08T15:26:04Z

internal/services/mssql/mssql_managed_database_resource.go

+				_, err := shortTermRetentionClient.CreateOrUpdate(ctx, id.ResourceGroup, id.ManagedInstanceName, id.DatabaseName, shortTermRetentionPolicy)
+				if err != nil {
+					return fmt.Errorf("updating Short Term Retention Policy for %s: %+v", id, err)
+				}


we can simplify this to:

Suggested change

_, err := shortTermRetentionClient.CreateOrUpdate(ctx, id.ResourceGroup, id.ManagedInstanceName, id.DatabaseName, shortTermRetentionPolicy)

if err != nil {

return fmt.Errorf("updating Short Term Retention Policy for %s: %+v", id, err)

}

if _, err := shortTermRetentionClient.CreateOrUpdate(ctx, id.ResourceGroup, id.ManagedInstanceName, id.DatabaseName, shortTermRetentionPolicy); err != nil {

return fmt.Errorf("updating Short Term Retention Policy for %s: %+v", id, err)

}

jackofallops · 2023-03-08T15:26:48Z

internal/services/mssql/mssql_managed_database_resource.go

+				_, err := shortTermRetentionClient.CreateOrUpdate(ctx, id.ResourceGroup, id.ManagedInstanceName, id.DatabaseName, shortTermRetentionPolicy)
+				if err != nil {
+					return fmt.Errorf("setting Short Term Retention Policy for %s: %+v", id, err)
+				}


This can be simplified to:

Suggested change

_, err := shortTermRetentionClient.CreateOrUpdate(ctx, id.ResourceGroup, id.ManagedInstanceName, id.DatabaseName, shortTermRetentionPolicy)

if err != nil {

return fmt.Errorf("setting Short Term Retention Policy for %s: %+v", id, err)

}

if _, err := shortTermRetentionClient.CreateOrUpdate(ctx, id.ResourceGroup, id.ManagedInstanceName, id.DatabaseName, shortTermRetentionPolicy); err != nil {

return fmt.Errorf("setting Short Term Retention Policy for %s: %+v", id, err)

}

jackofallops · 2023-03-08T15:28:55Z

internal/services/mssql/mssql_managed_database_resource.go

+
+		"long_term_retention_policy": helper.LongTermRetentionPolicySchema(),
+
+		"short_term_retention_days": {


Is there are default value for this returned from the API?

mbfrahry

LGTM!

github-actions · 2023-03-10T05:29:33Z

This functionality has been released in v3.47.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2023-04-11T02:09:22Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

catriona-m added 6 commits February 20, 2023 22:48

mssql database retention policies

35795ad

add update for retention policies

5d666ea

add retention docs

63b0499

make short_term_retention_days optional and fix client

b8fdd02

mssql database retention policies

af13d94

fix ltr model

25221c4

github-actions bot added documentation service/mssql Microsoft SQL Server size/L labels Mar 8, 2023

catriona-m requested a review from a team March 8, 2023 15:23

katbyte approved these changes Mar 8, 2023

View reviewed changes

mbfrahry requested changes Mar 8, 2023

View reviewed changes

github-actions bot added the waiting-response label Mar 8, 2023

jackofallops requested changes Mar 9, 2023

View reviewed changes

add default short_term_retention_days and simplify requests

04fcf03

github-actions bot removed the waiting-response label Mar 9, 2023

mbfrahry approved these changes Mar 9, 2023

View reviewed changes

mbfrahry merged commit 04964a4 into main Mar 9, 2023

mbfrahry deleted the cm/miltrpolicy branch March 9, 2023 23:17

mbfrahry added this to the v3.47.0 milestone Mar 9, 2023

mbfrahry added a commit that referenced this pull request Mar 9, 2023

Update CHANGELOG.md for #20845

be86b5f

github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_mssql_managed_database` - support for retention policies #20845

`azurerm_mssql_managed_database` - support for retention policies #20845

catriona-m commented Mar 8, 2023

katbyte left a comment

katbyte Mar 8, 2023

katbyte Mar 8, 2023

catriona-m Mar 9, 2023

mbfrahry left a comment

jackofallops left a comment

jackofallops Mar 8, 2023

jackofallops Mar 8, 2023

jackofallops Mar 8, 2023

mbfrahry left a comment

github-actions bot commented Mar 10, 2023

github-actions bot commented Apr 11, 2023

	* `short_term_retention_days` - (Optional) The backup retention period in days. This is how many days Point-in-Time Restore will be supported.
	* `short_term_retention_days` - (Optional) The backup retention period in days. This is how many days Point-in-Time Restore will be supported for.


		"long_term_retention_policy": helper.LongTermRetentionPolicySchema(),

		"short_term_retention_days": {

azurerm_mssql_managed_database - support for retention policies #20845

azurerm_mssql_managed_database - support for retention policies #20845

Conversation

catriona-m commented Mar 8, 2023

katbyte left a comment

Choose a reason for hiding this comment

katbyte Mar 8, 2023

Choose a reason for hiding this comment

katbyte Mar 8, 2023

Choose a reason for hiding this comment

catriona-m Mar 9, 2023

Choose a reason for hiding this comment

mbfrahry left a comment

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops Mar 8, 2023

Choose a reason for hiding this comment

jackofallops Mar 8, 2023

Choose a reason for hiding this comment

jackofallops Mar 8, 2023

Choose a reason for hiding this comment

mbfrahry left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 10, 2023

github-actions bot commented Apr 11, 2023

`azurerm_mssql_managed_database` - support for retention policies #20845

`azurerm_mssql_managed_database` - support for retention policies #20845