Feature Enhancement: Enable Backup Instance creation for Microsoft.DataProtection #3736
Conversation
mayankagg9722
requested review from
davefellows,
theunrepentantgeek,
matthchr,
babbageclunk and
super-harsh
as code owners
v2/samples/dataprotection/v1api/v1api20230101_backupvaultsbackupinstance.yaml
Outdated
Show resolved
Hide resolved
I have addressed all the comments. Merged with upstream main and regenerated and added test recordings. |
v2/samples/dataprotection/v1api20231101/refs/v1api20230501_extension.yaml
Outdated
Show resolved
Hide resolved
v2/samples/dataprotection/v1api20231101/refs/v1api20230501_extension.yaml
Outdated
Show resolved
Hide resolved
v2/samples/dataprotection/v1api20231101/v1api20231101_backupvaultsbackupinstance.yaml
Outdated
Show resolved
Hide resolved
v2/samples/dataprotection/v1api20231101/v1api20231101_backupvaultsbackupinstance.yaml
Outdated
Show resolved
Hide resolved
|
/ok-to-test sha=2433622 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
v2/api/dataprotection/customizations/backup_vaults_backup_instance_extensions.go
Show resolved
Hide resolved
| } | ||
|
|
||
| // give cluster msi access over snapshot rg for pv creation | ||
| clusterMSIRoleAssignmentAssignmentGUID, err := tc.Namer.GenerateUUID() |
There was a problem hiding this comment.
Consider moving these all reference resources to their own methods and return from there? Something like NewRoleAssignmentsForDataProtection...
There was a problem hiding this comment.
Thanks for the suggestions will make this improvement.
|
This fixes #2345 |
| log.V(Debug).Info(fmt.Sprintf("Protection Status is %q", protectionStatus)) | ||
|
|
||
| // return success for reconcilation if the state is non-retryable | ||
| if nonRetryableStates.Contains(protectionStatus) { |
There was a problem hiding this comment.
Would suggest inversing this logic here. We can always return success, if protectionStatus != "ProtectionError"
There was a problem hiding this comment.
that way, we're not checking the list each time during the reconcile.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/ok-to-test sha=e66c3fd |
What this PR does / why we need it:
This pull request introduces enhancement for Backup Instance creation for Microsoft.DataProtection. Additionally, we have added the necessary test recording that covers the creation, and deletion of these resources, namely Backup Instance with thorough validation.
Fixes #2345.
Special notes for your reviewer: Feature enhancement on top of already existing Microsoft.DataProtection CRDs.
How does this PR make you feel:
If applicable: