-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
provider - support for the recover_soft_deleted_backup_protected_vm
feature
#24157
provider - support for the recover_soft_deleted_backup_protected_vm
feature
#24157
Conversation
ad42736
to
ad0e160
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.
Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍
internal/services/recoveryservices/backup_protected_vm_resource.go
Outdated
Show resolved
Hide resolved
internal/services/recoveryservices/backup_protected_vm_resource.go
Outdated
Show resolved
Hide resolved
internal/services/recoveryservices/backup_protected_vm_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/recoveryservices/backup_protected_vm_resource.go
Outdated
Show resolved
Hide resolved
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 now!
internal/provider/features.go
Outdated
@@ -255,6 +255,21 @@ func schemaFeatures(supportLegacyTestSuite bool) *pluginsdk.Schema { | |||
}, | |||
}, | |||
|
|||
"recovery_services_vault": { |
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 plural
"recovery_services_vault": { | |
"recovery_services_vaults": { |
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 service seems to have a new issue (Azure/azure-rest-api-specs#27869) and needs workaround on #24978 |
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.
#24978 has been merged but i am still seeing test failures
Code updated and the tests pass now. I removed the change on |
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.
@@ -199,6 +205,12 @@ The `resource_group` block supports the following: | |||
|
|||
--- | |||
|
|||
The `recovery_services_vault` block supports the following: | |||
|
|||
* `recover_soft_deleted_backup_protected_vm` - (Optional) Should the `azurerm_backup_protected_vm` resource recover a Soft-Deleted protected VM? Defaults to `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.
I think this should default to false?
internal/provider/features.go
Outdated
"recover_soft_deleted_backup_protected_vm": { | ||
Type: pluginsdk.TypeBool, | ||
Optional: true, | ||
Default: 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.
this is going to change the existing behaviour?
Default: true, | |
Default: false, |
TBH these tests on main branch are also flakey.. |
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 doesn't seem to be a flakey test failure:
------- Stdout: -------
=== RUN TestAccBackupProtectedVm_updateBackupPolicyId
=== PAUSE TestAccBackupProtectedVm_updateBackupPolicyId
=== CONT TestAccBackupProtectedVm_updateBackupPolicyId
testcase.go:113: Step 1/5 error: Error running pre-apply refresh: exit status 1
Error: Duplicate provider configuration
on terraform_plugin_test.tf line 26:
26: provider "azurerm" {
A default (non-aliased) provider configuration for "azurerm" was already
given at terraform_plugin_test.tf:21,1-19. If multiple configurations are
required, set the "alias" argument for alternative configurations.
testing_new.go:79: Error retrieving state, there may be dangling resources: exit status 1
Error: Duplicate provider configuration
on terraform_plugin_test.tf line 26:
26: provider "azurerm" {
A default (non-aliased) provider configuration for "azurerm" was already
given at terraform_plugin_test.tf:21,1-19. If multiple configurations are
required, set the "alias" argument for alternative configurations.
--- FAIL: TestAccBackupProtectedVm_updateBackupPolicyId (5.44s)
FAIL
Should be good to merge once the test is fixed
uh...my fault. updated
|
azurerm_backup_protected_vm
- support recover soft deleted vmrecover_soft_deleted_backup_protected_vm
feature
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 📨
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. |
fix #24051
features expand