Compute Migration to Track2 SDK for Get-Azvm #28967
Conversation
…for Compute Management SDK
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the Get-AzVM cmdlet from the Track1 SDK (Microsoft.Azure.Management.Compute) to the Track2 SDK (Azure.ResourceManager.Compute v1.13.0), introduces a new ComputeClientTrack2 wrapper class for Track2 operations, and adds new scheduled events parameters to both Get-AzVM and Update-AzVM cmdlets. The PR also updates Azure.Core to version 1.50.0 across multiple projects and updates REST API specification files.
Key changes:
- Migration of
Get-AzVMto use Track2 SDK via newComputeClientTrack2helper class - Addition of
ScheduledEventsApiVersionandEnableAllInstancesDownparameters to VM cmdlets - Update of Azure.Core dependency to version 1.50.0 across 12 projects
Critical Issues Found:
- Missing AutoMapper configurations for Track2 SDK type conversions
- Scheduled events policy created but never applied to VM parameters in Update command
- Incorrect GitHub URLs in AutoRest README configuration
- Unused parameters in Get command implementation
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Compute/Compute/VirtualMachine/Operation/GetAzureVMCommand.cs | Migrates Get-AzVM to Track2 SDK; adds unused scheduled events parameters; contains commented-out Track1 code |
| src/Compute/Compute/VirtualMachine/Operation/UpdateAzureVMCommand.cs | Adds scheduled events parameters and Track2 update logic; critical bug where policy is not applied |
| src/Compute/Compute/Common/ComputeClientTrack2.cs | New Track2 SDK wrapper with authentication, VM operations, and logging support |
| src/Compute/Compute/Common/ComputeClientBaseCmdlet.cs | Adds ComputeClientTrack2 property to base cmdlet class |
| src/Compute/Compute/Compute.csproj | Adds Azure.ResourceManager.Compute v1.13.0 and updates Azure.Core to v1.50.0 |
| src/Compute/Compute/Compute.sln | New solution file (likely unnecessary per repository conventions) |
| src/Compute/Compute.Test/Compute.Test.csproj | Updates Azure.Core to v1.50.0 |
| src/Compute/Compute.Management.Sdk/README.md | Updates AutoRest input file URLs to reference newer API specs (contains incorrect URL formats) |
| src/Compute/Compute.Management.Sdk/Rest-api-specs/types.json | Updates to Azure common types v3 specification |
| src/Compute/Compute.Management.Sdk/Rest-api-specs/common.json | Updates to Azure common types with breaking enum name changes |
| src/Compute/Compute.Management.Sdk/Compute.Management.Sdk.csproj | Updates Azure.Core to v1.50.0 |
| src/Compute/Compute.Management.Sdk/Compute.Management.Sdk.sln | New solution file (likely unnecessary per repository conventions) |
| src/Compute/Compute.Helpers/Compute.Helpers.csproj | Updates Azure.Core to v1.50.0 |
| src/Compute/Compute.AlcWrapper/Compute.AlcWrapper.csproj | Updates Azure.Core to v1.50.0 |
| src/Network/Network/Network.csproj | Updates Azure.Core to v1.50.0 |
| src/Network/Network.Management.Sdk/Network.Management.Sdk.csproj | Updates Azure.Core to v1.50.0 |
| src/KeyVault/KeyVault/KeyVault.csproj | Updates Azure.Core to v1.50.0 |
| src/KeyVault/KeyVault.Management.Sdk/KeyVault.Management.Sdk.csproj | Updates Azure.Core to v1.50.0 |
| src/ManagedServiceIdentity/ManagedServiceIdentity/ManagedServiceIdentity.csproj | Updates Azure.Core to v1.50.0 |
| generated/Compute/Compute.Autorest/Az.Compute.csproj | Updates Azure.Core to v1.50.0 |
| generated/KeyVault/KeyVault.Autorest/Az.KeyVault.csproj | Updates Azure.Core to v1.50.0 |
| generated/ManagedServiceIdentity/ManagedServiceIdentity.Autorest/Az.ManagedServiceIdentity.csproj | Updates Azure.Core to v1.50.0 |
| tools/TestFx/TestFx.csproj | Updates Azure.Core to v1.50.0 |
| "allOf": [ | ||
| { | ||
| "$ref": "#/definitions/ResourceOriginal" | ||
| "$ref": "#/definitions/Resource" |
There was a problem hiding this comment.
The reference has been changed from #/definitions/ResourceOriginal to #/definitions/Resource. This change should be verified to ensure that: (1) the Resource definition exists in the updated schema and has the expected properties, and (2) this change is intentional and aligns with the Azure common types v3 specification. The corresponding removal of the ResourceOriginal definition should also be confirmed as correct.
|
|
||
| var psResult = ComputeAutoMapperProfile.Mapper.Map<PSVirtualMachine>(result); | ||
| if (result.Body != null) | ||
| VirtualMachineData data = vmResource.Data; |
There was a problem hiding this comment.
The variable data is assigned on line 188 but never used. It should be removed to improve code maintainability.
| VirtualMachineData data = vmResource.Data; |
| try | ||
| { | ||
| var resourceGroup = await GetResourceGroupAsync(resourceGroupName, cancellationToken); | ||
| var response = await resourceGroup.GetVirtualMachineAsync(vmName, cancellationToken:cancellationToken); |
There was a problem hiding this comment.
Inconsistent spacing in the named parameter. There's an extra space before the colon in cancellationToken:cancellationToken. It should be cancellationToken: cancellationToken (space after colon, not before) to follow C# conventions, or more commonly cancellationToken without the parameter name since it's clear from context.
| var response = await resourceGroup.GetVirtualMachineAsync(vmName, cancellationToken:cancellationToken); | |
| var response = await resourceGroup.GetVirtualMachineAsync(vmName, cancellationToken: cancellationToken); |
| null, | ||
| null); | ||
|
|
||
| return new ValueTask<AccessToken>(new AccessToken(accessToken.AccessToken, DateTimeOffset.UtcNow.AddHours(1))); |
There was a problem hiding this comment.
The token expiration is hardcoded to 1 hour from the current time (DateTimeOffset.UtcNow.AddHours(1)), but the actual token retrieved from AzureSession.Instance.AuthenticationFactory.Authenticate likely has its own expiration time. This could lead to the Track2 SDK thinking a token is valid when it has actually expired, or vice versa. The actual expiration time should be obtained from the access token response if available.
| return new ValueTask<AccessToken>(new AccessToken(accessToken.AccessToken, DateTimeOffset.UtcNow.AddHours(1))); | |
| // Use the actual expiration time from the access token if available | |
| DateTimeOffset expiresOn = accessToken.ExpiresOn; | |
| // Fallback to 1 hour if ExpiresOn is not set (for safety) | |
| if (expiresOn == default) | |
| { | |
| expiresOn = DateTimeOffset.UtcNow.AddHours(1); | |
| } | |
| return new ValueTask<AccessToken>(new AccessToken(accessToken.AccessToken, expiresOn)); |
| ArmCompute.VirtualMachinePatch vmPatch = ComputeAutoMapperProfile.Mapper.Map<ArmCompute.VirtualMachinePatch>(parameters); | ||
|
|
There was a problem hiding this comment.
The mapper is being used to convert Track1 VirtualMachine parameters to Track2 VirtualMachinePatch, but there's no evidence that mapping configuration exists for this conversion in ComputeAutoMapperProfile. The AutoMapper will likely fail at runtime or produce incomplete results since Track1 SDK types (Microsoft.Azure.Management.Compute.Models) and Track2 SDK types (Azure.ResourceManager.Compute.Models) have different structures and namespaces. You need to add appropriate AutoMapper configuration to handle the conversion between these SDK versions.
| ArmCompute.VirtualMachinePatch vmPatch = ComputeAutoMapperProfile.Mapper.Map<ArmCompute.VirtualMachinePatch>(parameters); | |
| // Manually map Track1 VirtualMachine parameters to Track2 VirtualMachinePatch | |
| var vmPatch = new ArmCompute.VirtualMachinePatch(); | |
| // Example property mappings (add more as needed) | |
| if (parameters.Tags != null) | |
| { | |
| vmPatch.Tags = new Dictionary<string, string>(); | |
| foreach (var tag in parameters.Tags) | |
| { | |
| vmPatch.Tags[tag.Key] = tag.Value; | |
| } | |
| } | |
| if (parameters.HardwareProfile != null) | |
| { | |
| vmPatch.HardwareProfile = new ArmCompute.HardwareProfile(); | |
| vmPatch.HardwareProfile.VmSize = parameters.HardwareProfile.VmSize; | |
| } | |
| if (parameters.OsProfile != null) | |
| { | |
| vmPatch.OsProfile = new ArmCompute.OSProfile(); | |
| vmPatch.OsProfile.ComputerName = parameters.OsProfile.ComputerName; | |
| vmPatch.OsProfile.AdminUsername = parameters.OsProfile.AdminUsername; | |
| // Do not map AdminPassword for patch | |
| } | |
| if (parameters.StorageProfile != null) | |
| { | |
| vmPatch.StorageProfile = new ArmCompute.StorageProfile(); | |
| // Map properties as needed, e.g. ImageReference, OsDisk, DataDisks, etc. | |
| if (parameters.StorageProfile.ImageReference != null) | |
| { | |
| vmPatch.StorageProfile.ImageReference = new ArmCompute.ImageReference | |
| { | |
| Publisher = parameters.StorageProfile.ImageReference.Publisher, | |
| Offer = parameters.StorageProfile.ImageReference.Offer, | |
| Sku = parameters.StorageProfile.ImageReference.Sku, | |
| Version = parameters.StorageProfile.ImageReference.Version | |
| }; | |
| } | |
| // Add more mappings as needed | |
| } | |
| if (parameters.NetworkProfile != null) | |
| { | |
| vmPatch.NetworkProfile = new ArmCompute.NetworkProfile(); | |
| // Map network interfaces if needed | |
| } | |
| // Add more property mappings as required for your scenario | |
| if (this.IsParameterBound(c => c.ScheduledEventsApiVersion) || this.IsParameterBound(c => c.EnableAllInstancesDown)) | |
| { | |
| var scheduledEventsPolicy = new ArmCompute.ScheduledEventsPolicy(); | |
| if (this.IsParameterBound(c => c.ScheduledEventsApiVersion)) | |
| { | |
| scheduledEventsPolicy.ScheduledEventsAdditionalPublishingTargetsEventGridAndResourceGraph = | |
| new ArmCompute.EventGridAndResourceGraph | |
| { | |
| IsEnabled = true, | |
| ScheduledEventsApiVersion = this.ScheduledEventsApiVersion | |
| }; | |
| } | |
| if (this.IsParameterBound(c => c.EnableAllInstancesDown)) | |
| { | |
| scheduledEventsPolicy.AllInstancesDown = new ArmCompute.AllInstancesDown | |
| { | |
| AutomaticallyApprove = this.EnableAllInstancesDown | |
| }; | |
| } | |
| vmPatch.ScheduledEventsPolicy = scheduledEventsPolicy; | |
| } | |
| if (this.IsParameterBound(c => c.AlignRegionalDisksToVMZone)) | |
| { | |
| if (vmPatch.StorageProfile == null) | |
| { | |
| vmPatch.StorageProfile = new ArmCompute.StorageProfile(); | |
| } | |
| vmPatch.StorageProfile.AlignRegionalDisksToVMZone = this.AlignRegionalDisksToVMZone; | |
| } | |
| // End manual mapping |
| Microsoft Visual Studio Solution File, Format Version 12.00 | ||
| # Visual Studio Version 17 | ||
| VisualStudioVersion = 17.5.2.0 | ||
| MinimumVisualStudioVersion = 10.0.40219.1 | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Compute", "Compute.csproj", "{9647CB21-5F2F-6929-9417-DAF5DB0FD617}" | ||
| EndProject | ||
| Global | ||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
| Debug|Any CPU = Debug|Any CPU | ||
| Release|Any CPU = Release|Any CPU | ||
| EndGlobalSection | ||
| GlobalSection(ProjectConfigurationPlatforms) = postSolution | ||
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| EndGlobalSection | ||
| GlobalSection(SolutionProperties) = preSolution | ||
| HideSolutionNode = FALSE | ||
| EndGlobalSection | ||
| GlobalSection(ExtensibilityGlobals) = postSolution | ||
| SolutionGuid = {E9250FC3-8ECB-4057-AC74-18EA469139E2} | ||
| EndGlobalSection | ||
| EndGlobal |
There was a problem hiding this comment.
Adding a new solution file (.sln) appears to be unnecessary as the project is already part of the main repository build system. Solution files are typically not added for individual modules in this repository. This file should likely be removed to maintain consistency with the repository structure.
| Microsoft Visual Studio Solution File, Format Version 12.00 | |
| # Visual Studio Version 17 | |
| VisualStudioVersion = 17.5.2.0 | |
| MinimumVisualStudioVersion = 10.0.40219.1 | |
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Compute", "Compute.csproj", "{9647CB21-5F2F-6929-9417-DAF5DB0FD617}" | |
| EndProject | |
| Global | |
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | |
| Debug|Any CPU = Debug|Any CPU | |
| Release|Any CPU = Release|Any CPU | |
| EndGlobalSection | |
| GlobalSection(ProjectConfigurationPlatforms) = postSolution | |
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | |
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Debug|Any CPU.Build.0 = Debug|Any CPU | |
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Release|Any CPU.ActiveCfg = Release|Any CPU | |
| {9647CB21-5F2F-6929-9417-DAF5DB0FD617}.Release|Any CPU.Build.0 = Release|Any CPU | |
| EndGlobalSection | |
| GlobalSection(SolutionProperties) = preSolution | |
| HideSolutionNode = FALSE | |
| EndGlobalSection | |
| GlobalSection(ExtensibilityGlobals) = postSolution | |
| SolutionGuid = {E9250FC3-8ECB-4057-AC74-18EA469139E2} | |
| EndGlobalSection | |
| EndGlobal |
| ValueFromPipeline = true)] | ||
| public String ResourceId { get; set; } | ||
|
|
||
| // In the parameter section, add: |
There was a problem hiding this comment.
The comment "// In the parameter section, add:" is unnecessary and appears to be a leftover from development. It should be removed.
| // In the parameter section, add: |
| { | ||
| AutomaticallyApprove = this.EnableAllInstancesDown | ||
| }; | ||
| } |
There was a problem hiding this comment.
The scheduledEventsPolicy is created and configured but never assigned to the VM parameters. After the if block on line 484, the scheduledEventsPolicy object is not used, meaning the scheduled events configuration will not be applied to the VM. You need to assign it to parameters.ScheduledEventsProfile.ScheduledEventsPolicy or similar property before calling the update operation.
| } | |
| } | |
| // Assign the policy to the parameters object | |
| if (parameters.ScheduledEventsProfile == null) | |
| { | |
| parameters.ScheduledEventsProfile = new ArmCompute.ScheduledEventsProfile(); | |
| } | |
| parameters.ScheduledEventsProfile.ScheduledEventsPolicy = scheduledEventsPolicy; |
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] There are excessive blank lines that should be removed to improve code consistency.
| Microsoft Visual Studio Solution File, Format Version 12.00 | ||
| # Visual Studio Version 17 | ||
| VisualStudioVersion = 17.5.2.0 | ||
| MinimumVisualStudioVersion = 10.0.40219.1 | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Compute.Management.Sdk", "Compute.Management.Sdk.csproj", "{4D5D485F-DB24-A55D-D626-74B523D84797}" | ||
| EndProject | ||
| Global | ||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
| Debug|Any CPU = Debug|Any CPU | ||
| Release|Any CPU = Release|Any CPU | ||
| EndGlobalSection | ||
| GlobalSection(ProjectConfigurationPlatforms) = postSolution | ||
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| EndGlobalSection | ||
| GlobalSection(SolutionProperties) = preSolution | ||
| HideSolutionNode = FALSE | ||
| EndGlobalSection | ||
| GlobalSection(ExtensibilityGlobals) = postSolution | ||
| SolutionGuid = {9BD3E99F-41C6-4076-BEEF-CAD959DCC74E} | ||
| EndGlobalSection | ||
| EndGlobal |
There was a problem hiding this comment.
Adding a new solution file (.sln) appears to be unnecessary as the project is already part of the main repository build system. Solution files are typically not added for individual modules in this repository. This file should likely be removed to maintain consistency with the repository structure.
| Microsoft Visual Studio Solution File, Format Version 12.00 | |
| # Visual Studio Version 17 | |
| VisualStudioVersion = 17.5.2.0 | |
| MinimumVisualStudioVersion = 10.0.40219.1 | |
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Compute.Management.Sdk", "Compute.Management.Sdk.csproj", "{4D5D485F-DB24-A55D-D626-74B523D84797}" | |
| EndProject | |
| Global | |
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | |
| Debug|Any CPU = Debug|Any CPU | |
| Release|Any CPU = Release|Any CPU | |
| EndGlobalSection | |
| GlobalSection(ProjectConfigurationPlatforms) = postSolution | |
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | |
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Debug|Any CPU.Build.0 = Debug|Any CPU | |
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Release|Any CPU.ActiveCfg = Release|Any CPU | |
| {4D5D485F-DB24-A55D-D626-74B523D84797}.Release|Any CPU.Build.0 = Release|Any CPU | |
| EndGlobalSection | |
| GlobalSection(SolutionProperties) = preSolution | |
| HideSolutionNode = FALSE | |
| EndGlobalSection | |
| GlobalSection(ExtensibilityGlobals) = postSolution | |
| SolutionGuid = {9BD3E99F-41C6-4076-BEEF-CAD959DCC74E} | |
| EndGlobalSection | |
| EndGlobal |
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.