[Backup] AFS VaultStandard #30531
Open
[Backup] AFS VaultStandard #30531
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
IannGeorges
requested review from
zhoxing-ms,
jsntcy and
yanzhudd
as code owners
️✔️AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| backup policy set | cmd backup policy set added parameter yes |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
|
microsoft-github-policy-service
bot
requested review from
yonzhan and
wangzelin007
IannGeorges
force-pushed
the
users/ianna/afsVaultPolicy
branch
from
20423fe to
de7c8b2
Compare
|
@yonzhan @zhoxing-ms Could you please re-trigger the pipeline, I want to download private build for bug bash |
zubairabid
suggested changes
Dec 27, 2024
| rp_tier = 'Unknown' | ||
|
|
||
| # Compare with the tier parameter and add to filtered list if matched | ||
| if rp_tier == tier: |
There was a problem hiding this comment.
are we just ignoring the tier parameter otherwise? We should inform the customer with a warning of some sorts, I think.
| existing_backup_management_type = existing_policy.properties.backup_management_type | ||
| new_backup_management_type = new_policy.properties.backup_management_type | ||
| if existing_backup_management_type != new_backup_management_type: | ||
| raise CLIError("BackupManagementType cannot be different than the existing type.") | ||
| # vault -> snapshot | ||
| if hasattr(existing_policy.properties, 'vault_retention_policy') and existing_policy.properties.vault_retention_policy is not None and hasattr(new_policy.properties, 'retention_policy') and new_policy.properties.retention_policy is not None: |
There was a problem hiding this comment.
this is being run for all policies, not just AFS. Is that desired?
| Please create a new policy for snapshot-only backups. | ||
| """) | ||
| # snapshot -> vault | ||
| if not yes and hasattr(existing_policy.properties, 'retention_policy') and existing_policy.properties.retention_policy is not None and hasattr(new_policy.properties, 'vault_retention_policy') and new_policy.properties.vault_retention_policy is not None: |
There was a problem hiding this comment.
If yes is passed as True, we never enter this path anyway. If the goal is to have yes skip the warning prompt, we need to remove the check from this line and add it lower down in 692: if yes or not prompt_y_n(warning_prompt).
| if not yes and hasattr(existing_policy.properties, 'retention_policy') and existing_policy.properties.retention_policy is not None and hasattr(new_policy.properties, 'vault_retention_policy') and new_policy.properties.vault_retention_policy is not None: | ||
| warning_prompt = ('Changing the backup tier keeps current snapshots as-is under the existing policy. Future backups will be stored in the vault with new retention settings.' | ||
| 'This action is irreversible and incurs additional costs. Switching from vault to snapshot requires reconfiguration.' | ||
| 'Learn more at https://learn.microsoft.com/en-us/azure/backup/azure-file-share-backup-overview?tabs=snapshot.') |
There was a problem hiding this comment.
Instruct the user here. Right now it's just a statement asking for a y/n input, and if they press "no" then the operation gets cancelled. Add a line that says "do you want to continue"
| @@ -395,7 +395,7 @@ def _get_key_url(self, **kwargs): | |||
| class AFSPolicyPreparer(AbstractPreparer, SingleValueReplacer): | |||
There was a problem hiding this comment.
- We don't need
instant_rp_daysanymore? - The code you've added for creating a new AFS policy seems fine, but it looks like the AFSPolicyPreparer already existed and presumably is used for some tests. Are you sure that your changes will not break other tests?
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
Description
Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.