azurerm_data_protection_backup_policy_postgresql: support target_copy_setting property

[sinbai]

When supporting targetDataStoreCopySettings property, it was found that TF is currently hard-coded for default azure retention rule and source data store partial value for the lifecycles(Additionally, there is the fact that for the default Azure retention rules/Azure retention rule, the lifecycle may have multiple items instead of the current hard-coded single item). The service team has confirmed that these properties are not immutable. I assume that these properties should be specified by the user.

Therefore, while submitting this PR to support targetDataStoreCopySettings, the original implementation (dependency) was changed and some properties were opened to users.

Test results:

• Bug Fix
• New Feature (ie adding a service, resource, or data source)
• Enhancement
• Breaking Change

[stephybun]

Can multiple life_cycle blocks be specified?

[sinbai]

yes

[stephybun]

What does this property do exactly?

[sinbai]

Specify when the backups are tiered across two or more selected data stores.

[katbyte]

that means this is a duration? should be call it copy_after_duration

[sinbai]

@katbyte thanks for your suggestion. copy_after really does not make it clear what this feature means. I assume that copy_option might be more suitable as the meaning of this feature is when to back up across two of more selected data stores. For example: Setting Immediate Copy means having a backup copy in both the standard vault and the archive vault simultaneously. Setting Copy On Expiry means moving the backup to vault-archive upon its expiry in vault-standard. WDYT?

[katbyte]

Thanks @sinbai - this looks like the new property won't take affect till 4.0?

is it possible to implement them now inline and support a graceful deprecation path for users instead of the hard breaking change in 4.0? as well allowing them to be used before?

[sinbai]

Thanks @sinbai - this looks like the new property won't take affect till 4.0?

is it possible to implement them now inline and support a graceful deprecation path for users instead of the hard breaking change in 4.0? as well allowing them to be used before?

Hi @katbyte thanks for your feedback and sounds like good suggestion. However, the newly added properties default_retention_rule and retention_rule.#.life_cycle should be required properties. If the original required attributes default_retention_duration and retention_rule.#.duration and the new two required are supported at the same time in the 3.0, these two sets of mutually exclusive attributes need to be modified to be optional. Then when the deprecated properties are deleted in the 4.0 , the new properties need to be changed to required. I assume this will be a breaking change (the new properties are changed from optional to required), so I assume this not appropriate, what do you think?

[katbyte]

Changing something from optional -> required during a major version is just fine! so lets make it optional & enable people using these properties now with the 4.0 flag flipping it to how things should behave in 4.0

[sinbai]

Changing something from optional -> required during a major version is just fine! so lets make it optional & enable people using these properties now with the 4.0 flag flipping it to how things should behave in 4.0

Hi @katbyte thanks for your time and feedback. I have updated the code. Could you please take another look? Test results are as follows:

[katbyte]

I think setting is redundant?

Suggested change

	A `target_copy_setting` block supports the following:
	A `target_copy` block supports the following:

[sinbai]

Fixed.

[katbyte]

and this might be better as

Suggested change

	* `copy_option` - (Required) Specifies when the backups are tiered across two or more selected data stores as a json encoded string. Changing this forces a new Backup Policy PostgreSQL to be created.
	* `option_json` - (Required) Specifies when the backups are tiered across two or more selected data stores as a json encoded string. Changing this forces a new Backup Policy PostgreSQL to be created.

?

[sinbai]

Fixed.

[katbyte]

are these not returned from the API? how come we are ignoring them?

[sinbai]

The API returns their values. However, TF supports both default_retention_duration (deprecated in v4.0) and default_retention_rule, which are two mutually exclusive properties in v3.0. These two properties corresponding to the same property of the API. Therefore, when importing resource in v3.0, I assume that we have no way of knowing whether the default_retention_duration or default_retention_rule is specified in the config, so I ignore them. Please see the following code in resourceDataProtectionBackupPolicyPostgreSQLRead func for details. Other than that, could you please let me know if there is a better solution?

[katbyte]

in the past when i have deprecated a property and replaced it with another property i've made them both computed and then just set them both in read so they have a valid property, and then only sent the value that exists/has been set in config off to the api in update/create.

is this possible with these fields to do that?

[sinbai]

Hi @katbyte thank you very much, I think this is a very good way. But there is a little problem. If the deprecated and replaced properties are set in read at the same time, then the following checks need to be deleted at the same time. Do you think it's okay to remove the check?

[sinbai]

Hi @katbyte a kind reminder in case you miss the above message.

[sinbai]

Hi @katbyte I have updated the code to fix the comments above. The test results are as follows, could you please take another look? Thanks for your time.

[github-actions]

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

[github-actions]

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

[stephybun]

@sinbai having taken another look, this will require some significant rework before this can go in.

Before we go down that path though I have a specific question which is:

Has the request to add support for target_copy come from the service team or is this our own initiative to improve coverage here?

I ask because when creating a Backup Policy for PostgreSQL in the Portal, the option target copy setting isn't exposed at all and there is no way to set that information in the policy currently.

See the JSON excerpt below of the Backup Policy created via the portal:

{
    "properties": {
        "policyRules": [
            {
                "lifecycles": [
                    {
                        "deleteAfter": {
                            "objectType": "AbsoluteDeleteOption",
                            "duration": "P6M"
                        },
                        "targetDataStoreCopySettings": [],
                        "sourceDataStore": {
                            "dataStoreType": "VaultStore",
                            "objectType": "DataStoreInfoBase"
                        }
                    }
                ],
                "isDefault": false,
                "name": "Monthly",
                "objectType": "AzureRetentionRule"

[sinbai]

@sinbai having taken another look, this will require some significant rework before this can go in.

Before we go down that path though I have a specific question which is:

Has the request to add support for target_copy come from the service team or is this our own initiative to improve coverage here?

I ask because when creating a Backup Policy for PostgreSQL in the Portal, the option target copy setting isn't exposed at all and there is no way to set that information in the policy currently.

See the JSON excerpt below of the Backup Policy created via the portal:

{
    "properties": {
        "policyRules": [
            {
                "lifecycles": [
                    {
                        "deleteAfter": {
                            "objectType": "AbsoluteDeleteOption",
                            "duration": "P6M"
                        },
                        "targetDataStoreCopySettings": [],
                        "sourceDataStore": {
                            "dataStoreType": "VaultStore",
                            "objectType": "DataStoreInfoBase"
                        }
                    }
                ],
                "isDefault": false,
                "name": "Monthly",
                "objectType": "AzureRetentionRule"

Hi @stephybun sorry for the late reply. Yes, support for target_copy is a request from the service team. Also, I confirmed with the service team the necessity of this feature support and just received a positive response from them. Regarding the issue of not being visible via Portal, in fact, the feature is visible to some users/ vaults, see the picture below, and they will internally check the reason behind this.

[sinbai]

@sinbai as 4.0 has been released could we update the PR with 5.0 flags?

addtionally i think the schema can be simplified

Hi @katbyte I have fixed the PR with 5.0 flags. Could you please take another look?

[katbyte]

@sinbai as per discussed on tuesday here is the breaking change & deprecation guide that this PR should follow: https://github.com/hashicorp/terraform-provider-azurerm/commits/main/contributing/topics/guide-breaking-changes.md

[sinbai]

@sinbai as per discussed on tuesday here is the breaking change & deprecation guide that this PR should follow: https://github.com/hashicorp/terraform-provider-azurerm/commits/main/contributing/topics/guide-breaking-changes.md

Hi @katbyte , thanks for your time. I'm assuming I've followed the breaking changes and deprecation guidelines for now, please let me know if I've missed/misunderstood something. Thank you!

Test results in TC: