Fix Compute Tools Issues#2687
Conversation
- COMP-02: VmDeleteCommand now uses DeleteVmAsync bool return to report 'not found' instead of always reporting success - COMP-03: Clarify --user-data requires Base64-encoded content; ARM API requires this format (docs/description update only) - COMP-04: vm/vmss update now correctly clears tags when --tags '' is passed; fix comma-separated tag delimiter mismatch in docs vs impl - COMP-06: DetermineOsType uses word-boundary token matching to avoid false positives (twin-ubuntu) and false negatives (VS Windows images like vs-2022-comm-latest-win11-n-gen2)
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple correctness issues in the Compute toolchain around VM/VMSS command behavior (delete not-found reporting, tag clearing semantics, and OS type detection from image identifiers), and adds/updates tests + documentation to reflect the corrected behavior.
Changes:
- Update
vm deleteto surface a “not found” message when the VM doesn’t exist (while keeping the operation idempotent / HTTP 200). - Make
vm update/vmss updatetreat--tags ''(and now bare--tags) as “clear all tags”, and align tag delimiter documentation with the implementation. - Improve
DetermineOsTypeimage matching to avoid false positives (e.g., “twin-ubuntu”) and correctly identify Windows Visual Studio images; add unit tests.
Invoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with write access 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.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Compute/tests/Azure.Mcp.Tools.Compute.Tests/Vmss/VmssUpdateCommandTests.cs | Adds test coverage for clearing VMSS tags via bare --tags. |
| tools/Azure.Mcp.Tools.Compute/tests/Azure.Mcp.Tools.Compute.Tests/Vm/VmUpdateCommandTests.cs | Adds tests for user-data pass-through and tag clearing semantics. |
| tools/Azure.Mcp.Tools.Compute/tests/Azure.Mcp.Tools.Compute.Tests/Vm/VmDeleteCommandTests.cs | Updates delete-not-found expectations (message + Success flag). |
| tools/Azure.Mcp.Tools.Compute/tests/Azure.Mcp.Tools.Compute.Tests/ComputeUtilitiesTests.cs | New unit tests for improved OS type detection. |
| tools/Azure.Mcp.Tools.Compute/src/Utilities/ComputeUtilities.cs | Refines OS detection logic to use token-based matching. |
| tools/Azure.Mcp.Tools.Compute/src/Services/ComputeService.cs | Implements “empty string clears tags” behavior for VM and VMSS patch operations. |
| tools/Azure.Mcp.Tools.Compute/src/Options/ComputeOptionDefinitions.cs | Updates tags/user-data option descriptions; changes --tags arity to allow bare option usage. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vmss/VmssUpdateCommand.cs | Treats bare --tags as an explicit clear request during validation/binding. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vm/VmUpdateCommand.cs | Same as VMSS: validation/binding recognizes explicit “clear tags”. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vm/VmDeleteCommand.cs | Uses service return value to shape message and Success flag. |
| servers/Azure.Mcp.Server/docs/azmcp-commands.md | Updates VM update parameter docs (tags delimiter + user-data details). |
| servers/Azure.Mcp.Server/changelog-entries/1779264486206.yaml | Adds a changelog entry describing the bug fixes. |
haagha
left a comment
There was a problem hiding this comment.
VMSS delete has the same behaviour - can we change that too?
Fixed multiple bugs in Compute VM/VMSS commands:
-
vm deletenow correctly reports 'not found' when the target VM does not exist instead of always reporting success.-
vm updateandvmss updatenow correctly clear all tags when--tags ''is passed; fixed comma-separated tag delimiter mismatch between documentation and implementation.-
DetermineOsTypenow uses word-boundary token matching: no longer misidentifies images with 'win' mid-word (e.g., 'twin-ubuntu') as Windows.