-
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
New resource: azurerm_netapp_backup_vault
and azurerm_netapp_backup_policy
#27188
Conversation
… snapshot_directory_visible
…test provider block
The unit tests failing below have nothing to do with my changes. |
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 @paulomarquesc, sorry for the delay in this review. Overall it looks good but it looks like underlying sdk got swapped out from underneath this PR and that'll need to be fixed.
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 @paulomarquesc, I've taken another pass and it overall looks to be in a good place but I've left quite a few comments to clean up some of the messaging and a few other design cleanups that we should do
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 @paulomarquesc, this is looking better! I just left a note around a missing nil check and then moving the _maintainers/readme file into the main netapp
folder while also cleaning up some of the formatting
} | ||
``` | ||
|
||
This is because some operations return from regular SDK polling as completed but due to several factors it is still in progress (e.g. ARM caching, software and hardware layer sync delays, etc.). These wait functions are necessary and should not be removed. |
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 formatting here got weird (some words have colors) and I'm worried it'll cause issues when people view the file? I believe it's because of the above code block being tabbed over but you'll have to mess with it and see
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.
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.
|
||
In our tests we have added this block so the tests can delete the resources, but in production customers should not allow deletion of volumes by default. | ||
|
||
```hcl |
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.
If formatting the above code block works, we'll want to do the same with this code block
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!
azurerm_netapp_backup_vault
and azurerm_netapp_backup_policy
azurerm_netapp_backup_vault
and azurerm_netapp_backup_policy
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. |
Community Note
Description
This PR consists of:
netapp_backup_vault
andnetapp_backup_policy
netapp
feature configuration block focused on safety as follows:prevent_volume_destruction
- Set totrue
by default, prevents a very common occurrence for Azure NetApp customers losing data due to volume destruction. This will change current behavior of volume destruction, and they will fail destruction if not set but this is required to be set now since we have many Azure customers (for various reasons) losing their volumes and for Azure NetApp Files, this is a non-recoverable disaster.delete_backups_on_backup_vault_destroy
- Set tofalse
by default, this will protect customers from having their backups destroyed, making sure customers can restore their data.prevent_volume_destruction
was added and set tofalse
to all acceptance tests that requires volume resource to allow it to clean up the test and complete it successfully2024-03-01
snapshot_directory_visible
volume configuration preventing acceptance tests for CRR to completePR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_netapp_backup_vault
- add support for backup vaults that are the parents of backups and it is associated with a volume.azurerm_netapp_backup_policy
- a backup policy to assign to a volume that indicates that volume will start to have internally scheduled backups and what is the retention policy.This is a (please select all that apply):
Related Issue(s)
Fixes #16242