-
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
add resource "azurerm_data_protection_backup_instance_postgresql" #12220
add resource "azurerm_data_protection_backup_instance_postgresql" #12220
Conversation
website/docs/r/data_protection_backup_instance_postgresql.html.markdown
Outdated
Show resolved
Hide resolved
@katbyte Hi, I added a commit to rename policy_id to backup_policy_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.
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
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. |
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.
@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
cb4841a
to
0e49e7f
Compare
@katbyte , I have fixed it in resource and removed the sleeps from test. |
azurerm/internal/services/dataprotection/data_protection_backup_instance_postgresql_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/dataprotection/data_protection_backup_instance_postgresql_resource.go
Show resolved
Hide resolved
hi tom, I added a commit according to your suggestion, thanks! |
bc9d313
to
153ba79
Compare
c6e23a5
to
893e608
Compare
Hi @katbyte , I have added a commit according to Tom's suggestion, please take another look, thanks! |
@katbyte hi, tc tests are passed. |
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 @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!
"resource_group_name": azure.SchemaResourceGroupName(), | ||
|
||
"location": location.Schema(), | ||
|
||
"vault_name": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
}, |
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.
rather than passing these two fields in we can replace this with the Vault ID and pass that in instead:
"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 | |
}, |
resourceGroup := d.Get("resource_group_name").(string) | ||
vaultName := d.Get("vault_name").(string) |
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.
which means we can then parse the ID to get these elements
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), | ||
} |
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.
this should be using the remaining time from the context, since these timeouts will start at different times otherwise
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), | |
} |
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 👍
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 @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!
website/docs/r/data_protection_backup_instance_postgresql.html.markdown
Outdated
Show resolved
Hide resolved
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! |
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. |
The tests are listed as the following.