Add azurebackup policy update command to Azure Backup toolset#2452
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new azurebackup policy update command to the Azure Backup toolset to update an existing RSV backup policy’s schedule time and/or daily retention days, with explicit rejection for DPP vaults.
Changes:
- Introduces
PolicyUpdateCommand+PolicyUpdateOptions, wires command into DI/command groups, and adds source-gen JSON context support. - Implements RSV update behavior in
RsvBackupOperationsvia fetch–mutate–save (GetAsync+CreateOrUpdateAsync) and exposes it through service interfaces. - Adds unit + live test coverage and updates server docs/tool mapping/changelog entries.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.AzureBackup/src/Commands/Policy/PolicyUpdateCommand.cs | New command implementation and error mapping for policy update. |
| tools/Azure.Mcp.Tools.AzureBackup/src/Options/Policy/PolicyUpdateOptions.cs | New options model for policy update inputs. |
| tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs | RSV implementation of policy update (fetch-modify-save + helper). |
| tools/Azure.Mcp.Tools.AzureBackup/src/Services/IRsvBackupOperations.cs | Adds RSV update method contract. |
| tools/Azure.Mcp.Tools.AzureBackup/src/Services/IAzureBackupService.cs | Adds service-level update policy contract. |
| tools/Azure.Mcp.Tools.AzureBackup/src/Services/AzureBackupService.cs | Routes update to RSV and rejects DPP updates. |
| tools/Azure.Mcp.Tools.AzureBackup/src/Commands/AzureBackupJsonContext.cs | Adds source-gen JSON support for update command result. |
| tools/Azure.Mcp.Tools.AzureBackup/src/AzureBackupSetup.cs | Registers the new command in DI and the policy command group. |
| tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/Policy/PolicyUpdateCommandTests.cs | New unit tests for command behavior and error handling. |
| tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/AzureBackupSetupTests.cs | Verifies policy update command registration. |
| tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.LiveTests/AzureBackupCommandTests.cs | Adds live tests for RSV updates and DPP rejection. |
| tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.LiveTests/assets.json | Updates recorded-test tag. |
| servers/Azure.Mcp.Server/src/Resources/consolidated-tools.json | Exposes azurebackup_policy_update in consolidated tool mapping. |
| servers/Azure.Mcp.Server/docs/e2eTestPrompts.md | Adds e2e prompts for the new command. |
| servers/Azure.Mcp.Server/docs/azmcp-commands.md | Adds command reference entry for azmcp azurebackup policy update. |
| servers/Azure.Mcp.Server/changelog-entries/1776767571721.yaml | Adds changelog entry for the new feature. |
|
Addressed all 3 Copilot review comments in the latest push: Comment 1 (scheduleTime/dailyRetentionDays silently treated as null): Comment 2 (all InvalidOperationException mapped to DPP message): Comment 3 (missing default case in UpdatePolicyScheduleAndRetention): Additional (cross-checked with PR #2441 feedback):
All 346 unit tests pass (15 policy update tests now). All 49 live tests pass in playback. Format clean. |
jongio
left a comment
There was a problem hiding this comment.
Two items from my previous review are still open after the latest push. Replied on both threads with specifics.
-
GetStatusCodestill has noInvalidOperationExceptionmapping - DPP and unsupported-policy-type errors return 500 instead of 400. The unit test asserts the wrong status code. -
scheduleAppliedflag is set in the wrong place in the IaasVM and FileShare branches - the retention sync block sets it even when the actual schedule policy wasn't updated. VmWorkload branch is correct and can be used as reference.
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jongio
left a comment
There was a problem hiding this comment.
Addresses my previous feedback cleanly:
- GetStatusCode now maps InvalidOperationException to BadRequest - both DPP-not-supported and unsupported-policy-type errors return 400 instead of 500.
- Removed the misplaced scheduleApplied = true from the IaasVM and FileShare retention sync blocks. Schedule tracking is now scoped correctly.
- Test assertions updated to match.
- est-resources-post.ps1 key casing fix should resolve the live test failure.
One action item: live tests need to be re-triggered to verify the DeploymentOutputs key casing fix works in CI.
|
Hi @anannya03 - the live test failure at the Test Resource step was caused by a case mismatch in test-resources-post.ps1: the script was reading DeploymentOutputs (PascalCase) but the pipeline sets deploymentOutputs (camelCase). Fixed in commit 9c34fd8. All review threads from both Copilot and @jongio are now resolved. Could someone please re-trigger live tests to verify the fix? |
jongio
left a comment
There was a problem hiding this comment.
Addresses all my previous feedback. The four items I flagged are fixed and the threads are resolved:
- GetStatusCode maps InvalidOperationException to BadRequest (both DPP and unsupported-policy-type cases)
- scheduleApplied/retentionApplied tracking prevents false-positive success
- scheduleApplied no longer set inside the retention sync blocks for IaasVM and FileShare
- Default case throws for unsupported policy types
One blocker before merge: there are merge conflicts on this branch that need rebasing.
9c34fd8 to
7afb4be
Compare
jongio
left a comment
There was a problem hiding this comment.
Clean implementation following the established AzureBackup command patterns. Error handling is solid - GetStatusCode maps both ArgumentException and InvalidOperationException to BadRequest, and GetErrorMessage has specific messages for DPP rejection vs generic IOEx.
The scheduleApplied/retentionApplied tracking correctly catches unsupported schedule/retention types at the end of UpdatePolicyScheduleAndRetention. Tests cover the key paths including DPP rejection, invalid inputs, schedule-only and retention-only updates.
One item to add before merge - see inline comment.
7afb4be to
accefe2
Compare
jongio
left a comment
There was a problem hiding this comment.
All prior feedback addressed. The rebase pulled in #2505 (telemetry tags for all AzureBackup commands), so PolicyUpdateCommand now includes the AddVaultTags call I flagged. The scheduleApplied/retentionApplied tracking, input validation, error handling, and test coverage all look solid. No new issues.
accefe2 to
63ba676
Compare
bf7faee to
6bb3a0a
Compare
jongio
left a comment
There was a problem hiding this comment.
Reviewed the rebase changes (HEAD moved from accefe2 to 6bb3a0a). Two new commits since my last review:
- CommandMetadata attribute applied to PolicyUpdateCommand (per anannya03's request) - values look correct: Destructive=true, Idempotent=true, OpenWorld=false, ReadOnly=false.
- Live tests re-recorded after rebase, retention test updated to increase rather than decrease (handles immutable vault constraints).
All prior feedback remains addressed. No new issues found.
…case, README update
…ploymentOutputs casing
…r immutable vault
a71f364 to
40ce45a
Compare
jongio
left a comment
There was a problem hiding this comment.
The merge of main brought in the AddCommand<T> API migration (#2538/#2539/#2540), but the policy group wasn't updated to match. The private RegisterCommand helper was deleted, so the three RegisterCommand<> calls on lines 78-80 won't compile - that's why all build checks are failing.
Also, the policy group description on line 76 still has an em-dash while all other descriptions were updated to hyphens.
Summary
Adds a new
azurebackup policy updatecommand that modifies an existing RSV backup policy's schedule time and daily retention settings. Supports IaasVM, SQL/SAP HANA (VmWorkload), and FileShare policy types. DPP vaults return a clear error since they do not support policy update.Changes
New Tool:
azurebackup policy updateRSV implementation:
GetAsync, mutates schedule/retention in-place on the existing properties object (preserving all other fields like PolicyId, WorkloadType, etc.)IaasVmProtectionPolicy), VmWorkload SQL/HANA (VmWorkloadProtectionPolicyFull sub-policy), and FileShare (FileShareProtectionPolicy)CreateOrUpdateAsyncwith the modified policy dataSucceededwith no-op message when no changes specifiedDPP implementation:
InvalidOperationExceptionwith clear message that DPP does not support policy updateFiles Added (3)
Files Modified (13)
UpdatePolicyAsyncto service contractInvalidOperationExceptionfor DPPUpdatePolicyScheduleAndRetentionhelperPolicyUpdateCommandin DI + policy command groupPolicyUpdateCommandResultazurebackup_policy_updatetoupdate_azure_backup_settingsTest Results
azurebackup_policy_updateranks Update README.md #1 for both test prompts (0.664, 0.689)Invoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with
writeaccess to the repo need to validate the contents of this PR before leaving a comment with the text/azp run mcp - pullrequest - live. This will trigger the necessary livetest workflows to complete required validation.