Skip to content
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

add resource "azurerm_data_protection_backup_instance_postgresql" #12220

Conversation

ms-henglu
Copy link
Contributor

The tests are listed as the following.

=== RUN   TestAccDataProtectionBackupInstancePostgreSQL_basic
=== PAUSE TestAccDataProtectionBackupInstancePostgreSQL_basic
=== CONT  TestAccDataProtectionBackupInstancePostgreSQL_basic
--- PASS: TestAccDataProtectionBackupInstancePostgreSQL_basic (501.75s)
=== RUN   TestAccDataProtectionBackupInstancePostgreSQL_requiresImport
=== PAUSE TestAccDataProtectionBackupInstancePostgreSQL_requiresImport
=== CONT  TestAccDataProtectionBackupInstancePostgreSQL_requiresImport
--- PASS: TestAccDataProtectionBackupInstancePostgreSQL_requiresImport (577.23s)
=== RUN   TestAccDataProtectionBackupInstancePostgreSQL_complete
=== PAUSE TestAccDataProtectionBackupInstancePostgreSQL_complete
=== CONT  TestAccDataProtectionBackupInstancePostgreSQL_complete
--- PASS: TestAccDataProtectionBackupInstancePostgreSQL_complete (490.25s)
=== RUN   TestAccDataProtectionBackupInstancePostgreSQL_update
=== PAUSE TestAccDataProtectionBackupInstancePostgreSQL_update
=== CONT  TestAccDataProtectionBackupInstancePostgreSQL_update
--- PASS: TestAccDataProtectionBackupInstancePostgreSQL_update (731.34s)

@ms-henglu
Copy link
Contributor Author

@katbyte Hi, I added a commit to rename policy_id to backup_policy_id

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ms-henglu - looks like we have a test failure:

------- Stdout: -------
=== RUN   TestAccDataProtectionBackupInstancePostgreSQL_update
=== PAUSE TestAccDataProtectionBackupInstancePostgreSQL_update
=== CONT  TestAccDataProtectionBackupInstancePostgreSQL_update
    testing.go:620: Step 3/6 error: Error running apply: exit status 1
        
        Error: creating/updating DataProtection BackupInstance ("Backup Instance: (Name \"acctest-dbi-210617181328535523\" / Backup Vault Name \"acctest-dataprotection-vault-210617181328535523\" / Resource Group \"acctest-dataprotection-210617181328535523\")"): dataprotection.BackupInstancesClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="UserErrorDppDatasourceConflictingOperation" Message="Cannot trigger the operation as another operation is already in progress on this datasource." AdditionalInfo=[{"info":{"code":"UserErrorDppDatasourceConflictingOperation","details":null,"innerError":null,"isRetryable":false,"isUserError":false,"message":"Cannot trigger the operation as another operation is already in progress on this datasource.","properties":{"ActivityId":"724727f8-2fd5-41a4-be89-68b354738a06"},"recommendedAction":["Please try again after some time."],"target":""},"type":"UserFacingError"}]
        
          with azurerm_data_protection_backup_instance_postgresql.test,
          on terraform_plugin_test.tf line 67, in resource "azurerm_data_protection_backup_instance_postgresql" "test":
          67: resource "azurerm_data_protection_backup_instance_postgresql" "test" {
        
    testing_new.go:21: WARNING: destroy failed, so remote objects may still exist and be subject to billing
    testing_new.go:21: failed to destroy: exit status 1
        
        Error: deleting DataProtection BackupPolicy ("Backup Policy: (Name \"acctest-dp-210617181328535523\" / Backup Vault Name \"acctest-dataprotection-vault-210617181328535523\" / Resource Group \"acctest-dataprotection-210617181328535523\")"): dataprotection.BackupPoliciesClient#Delete: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="UserErrorDppPolicyObjectInUse" Message="Cannot delete the backup policy as one or more backup instances have been configured for protection with this policy." AdditionalInfo=[{"info":{"code":"UserErrorDppPolicyObjectInUse","details":null,"innerError":null,"isRetryable":false,"isUserError":false,"message":"Cannot delete the backup policy as one or more backup instances have been configured for protection with this policy.","properties":{"ActivityId":"b5d9b669-361d-40c8-bf22-aa79589abb05"},"recommendedAction":["Ensure that no backup instances are configured for protection with this backup policy and then try deleting the policy."],"target":""},"type":"UserFacingError"}]
        
        
--- FAIL: TestAccDataProtectionBackupInstancePostgreSQL_update (505.43s)
FAIL

------- Stderr: -------
2021/06/17 18:13:28 [DEBUG] not using binary driver name, it's no longer needed
2021/06/17 18:13:28 [DEBUG] not using binary driver name, it's no longer needed

@ms-henglu
Copy link
Contributor Author

Thanks @katbyte for code review.

I've added a commit to fix the test. The cause is that instance will add protection on policy, and it can't be updated unless this operation is finished, usually it takes a very short time.

@ms-henglu ms-henglu requested a review from katbyte June 18, 2021 02:15
@katbyte katbyte modified the milestones: v2.64.0, v2.65.0 Jun 18, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ms-henglu - instead of hacking the test to fix it, can we change the code to wait for the operation to finish and remove the sleeps from the tests? thanks

@ms-henglu ms-henglu force-pushed the branch-210514-add-data-protection-resources-embeded-sdk-instance-postgresql branch from cb4841a to 0e49e7f Compare June 22, 2021 04:06
@ms-henglu
Copy link
Contributor Author

@katbyte , I have fixed it in resource and removed the sleeps from test.

@ms-henglu
Copy link
Contributor Author

hi tom, I added a commit according to your suggestion, thanks!

@ms-henglu ms-henglu force-pushed the branch-210514-add-data-protection-resources-embeded-sdk-instance-postgresql branch from bc9d313 to 153ba79 Compare June 22, 2021 15:38
@ms-henglu ms-henglu force-pushed the branch-210514-add-data-protection-resources-embeded-sdk-instance-postgresql branch from c6e23a5 to 893e608 Compare June 25, 2021 00:06
@ms-henglu
Copy link
Contributor Author

Hi @katbyte , I have added a commit according to Tom's suggestion, please take another look, thanks!

@ms-henglu
Copy link
Contributor Author

@katbyte hi, tc tests are passed.
image

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @ms-henglu

Thanks for pushing those changes

I've taken a look through and left a few other comments inline but if we can fix those up then this should otherwise be good to go 👍

Thanks!

Comment on lines 51 to 59
"resource_group_name": azure.SchemaResourceGroupName(),

"location": location.Schema(),

"vault_name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than passing these two fields in we can replace this with the Vault ID and pass that in instead:

Suggested change
"resource_group_name": azure.SchemaResourceGroupName(),
"location": location.Schema(),
"vault_name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"location": location.Schema(),
"vault_id": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validate.VaultID
},

Comment on lines 83 to 84
resourceGroup := d.Get("resource_group_name").(string)
vaultName := d.Get("vault_name").(string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which means we can then parse the ID to get these elements

Comment on lines 140 to 146
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{string(dataprotection.ConfiguringProtection), string(dataprotection.UpdatingProtection)},
Target: []string{string(dataprotection.ProtectionConfigured)},
Refresh: policyProtectionStateRefreshFunc(ctx, client, id),
MinTimeout: 1 * time.Minute,
Timeout: d.Timeout(pluginsdk.TimeoutCreate),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be using the remaining time from the context, since these timeouts will start at different times otherwise

Suggested change
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{string(dataprotection.ConfiguringProtection), string(dataprotection.UpdatingProtection)},
Target: []string{string(dataprotection.ProtectionConfigured)},
Refresh: policyProtectionStateRefreshFunc(ctx, client, id),
MinTimeout: 1 * time.Minute,
Timeout: d.Timeout(pluginsdk.TimeoutCreate),
}
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("context had no deadline")
}
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{string(dataprotection.ConfiguringProtection), string(dataprotection.UpdatingProtection)},
Target: []string{string(dataprotection.ProtectionConfigured)},
Refresh: policyProtectionStateRefreshFunc(ctx, client, id),
MinTimeout: 1 * time.Minute,
Timeout: time.Until(deadline),
}

@ms-henglu
Copy link
Contributor Author

Hi Tom, I have fixed according to your suggestions, please take another look, thanks!

And here's tc tests.
image

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

katbyte added a commit that referenced this pull request Jun 25, 2021
@ms-henglu ms-henglu requested a review from tombuildsstuff June 25, 2021 04:21
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @ms-henglu

Thanks for pushing those changes.

I've taken a look through and left one comment inline (which I'll push a commit for) - but this otherwise LGTM 👍

Thanks!

@tombuildsstuff tombuildsstuff merged commit 67ba475 into hashicorp:master Jun 25, 2021
@github-actions
Copy link

This functionality has been released in v2.65.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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants