Dev/bhbrahma/gallery managed identity#29381
Conversation
…dlets Co-authored-by: audreyttt <225061541+audreyttt@users.noreply.github.com>
…/github.com/Azure/azure-powershell into dev/bhbrahma/galleryManagedIdentity
| 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
Note
Copilot was unable to run its full agentic suite in this review.
Adds managed identity support to Azure Compute Gallery cmdlets by introducing new identity-related parameters and surfacing identity in output models.
Changes:
- Added
-EnableSystemAssignedIdentityand-UserAssignedIdentitytoNew-AzGalleryandUpdate-AzGallery. - Exposed
IdentityonPSGalleryand updated help docs with new syntax/examples. - Added scenario tests for system-assigned identity and fixed
Update-AzGalleryto return the updated gallery result.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Compute/Compute/Generated/Gallery/GalleryCreateOrUpdateMethod.cs | Implements new identity parameters, sets gallery identity, and fixes Update cmdlet result handling |
| src/Compute/Compute/Generated/Models/PSGallery.cs | Adds Identity to the PowerShell output model |
| src/Compute/Compute/help/New-AzGallery.md | Documents new identity parameters and examples for gallery creation |
| src/Compute/Compute/help/Update-AzGallery.md | Documents new identity parameters and update examples |
| src/Compute/Compute.Test/ScenarioTests/GalleryTests.ps1 | Adds scenario scripts validating system-assigned identity create/update flows |
| src/Compute/Compute.Test/ScenarioTests/GalleryTests.cs | Wires new scenario scripts into test runner |
| src/Compute/Compute/ChangeLog.md | Notes the new parameters and output model changes for the upcoming release |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
refactored the order in which the tests appeared in the source files
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…signedIdentity to Update-AzGallery
…hod.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…hod.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…hod.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on this feedback |
| var newIdentityIds = new HashSet<string>(this.UserAssignedIdentity, StringComparer.OrdinalIgnoreCase); | ||
| galleryUpdate.Identity.UserAssignedIdentities = new Dictionary<string, UserAssignedIdentitiesValue>(); | ||
|
|
There was a problem hiding this comment.
newIdentityIds is case-insensitive, but galleryUpdate.Identity.UserAssignedIdentities is created with the default (case-sensitive) comparer. If an existing identity key differs only by casing from a newly provided ID, the PATCH payload can end up retaining the old key (not nulled) and adding the new key, effectively duplicating the same identity. Use a case-insensitive dictionary for UserAssignedIdentities and normalize/resolve casing consistently when comparing existing vs requested IDs.
…hod.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if (existingKeys != null) | ||
| { | ||
| foreach (var existingKey in existingKeys) | ||
| { | ||
| if (!newIdentityIds.Contains(existingKey)) | ||
| { | ||
| galleryUpdate.Identity.UserAssignedIdentities[existingKey] = null; |
There was a problem hiding this comment.
The replace-user-assigned logic compares existing keys case-insensitively, but then adds desired IDs using whatever casing the user passed. If the same identity is already present with different casing, it won’t be nulled out and may end up duplicated in the PATCH payload/resource. Normalize keys against existing identities (or use a case-insensitive dictionary and canonicalize to existing keys) so replacement is deterministic.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
audreyttt
left a comment
There was a problem hiding this comment.
LGTM other than two comments
| Accept wildcard characters: False | ||
| ``` | ||
|
|
||
| ### -RemoveAllUserAssignedIdentity |
There was a problem hiding this comment.
Can you move this to ensure alphabetical ordering of the parameters
|
Hi @D1v38om83r , Please fix failed test cases, you can verify test results locally before pushing |
audreyttt
left a comment
There was a problem hiding this comment.
Because update is now a patch not a put, there is also an existing test failing that needs to be updated and re-recorded.
|
|
||
| if (this.ParameterSetName == "ObjectParameter") | ||
| { | ||
| ComputeAutomationAutoMapperProfile.Mapper.Map<PSGallery, Gallery>(this.InputObject, gallery); |
There was a problem hiding this comment.
A logic error is introduced here now. When Update-AzGallery -InputObject $gallery is used, the InputObject is mapped into the local Gallery variable, but the PATCH payload (GalleryUpdate) is only populated from explicitly-bound cmdlet parameters (via IsParameterBound checks). Since the user modified the PSGallery object directly rather than passing individual -Description, -Tag, etc. parameters, those checks all return False and GalleryUpdate is sent nearly empty.
| if (this.Share.IsPresent || this.Community.IsPresent || this.Reset.IsPresent) | ||
| { | ||
| GallerySharingProfileClient.Update(resourceGroupName, galleryName, sharingUpdate); | ||
| result = GalleriesClient.Get(ResourceGroupName, galleryName, "Permissions"); | ||
| result = GalleriesClient.Get(resourceGroupName, galleryName, "Permissions"); | ||
| } | ||
| else | ||
| { | ||
| GalleriesClient.CreateOrUpdate(resourceGroupName, galleryName, gallery); | ||
| result = GalleriesClient.Update(resourceGroupName, galleryName, galleryUpdate); | ||
| } |
There was a problem hiding this comment.
When -Share, -Community, or -Reset is present, the cmdlet only calls GallerySharingProfileClient.Update(...) and then does a GET, skipping GalleriesClient.Update(...). That means identity changes (and other non-sharing updates in galleryUpdate, like tags/description) will be silently ignored if a user combines them with sharing-related switches. Consider either (1) validating and throwing a clear error when sharing switches are combined with identity/other update parameters, or (2) applying both updates (sharing update + GalleriesClient.Update) in a deterministic order so the cmdlet honors all provided parameters.
Adds managed identity support to
New-AzGalleryandUpdate-AzGallery, and exposesIdentityin theGet-AzGalleryoutput object.Changes
PSGallery.cs— AddedIdentityproperty (GalleryIdentity); AutoMapper picks it up automatically from the SDK modelGalleryCreateOrUpdateMethod.csNew-AzGallery: new parameters-EnableSystemAssignedIdentity(SwitchParameter) and-UserAssignedIdentity(string[])Update-AzGallery: same two parameters plus-DisableSystemAssignedIdentity(SwitchParameter) and-RemoveAllUserAssignedIdentity(SwitchParameter) for removing identitiesSystemAssigned,UserAssigned, orSystemAssigned, UserAssignedUpdate-AzGallerymerges with the existing identity state on the resource — adding-EnableSystemAssignedIdentitypreserves existing user-assigned identities and vice versa-EnableSystemAssignedIdentity/-DisableSystemAssignedIdentityand-UserAssignedIdentity/-RemoveAllUserAssignedIdentitycannot be used togetherUsage
Parameter naming follows the repo's managed identity best practices.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
1n8vsblobprodwus2184.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)6yfvsblobprodwus2121.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)7t8vsblobprodwus2168.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)84hvsblobprodwus2148.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)8wdvsblobprodwus2137.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)9yyvsblobprodwus2157.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)b53vsblobprodwus2154.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)ba0vsblobprodwus2130.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)balvsblobprodwus2129.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)cbvvsblobprodwus2131.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)cffvsblobprodwus218.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)d94vsblobprodwus2119.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)ezcvsblobprodwus2170.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)faxvsblobprodwus2122.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)g3xvsblobprodwus2151.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)i01vsblobprodwus216.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)isvvsblobprodwus2147.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)jhgvsblobprodwus2167.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)k4pvsblobprodwus2140.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)mt2vsblobprodwus2110.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)nudvsblobprodwus214.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)p2tvsblobprodwus2189.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)tn2vsblobprodwus2124.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)wlnvsblobprodwus2188.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)x0dvsblobprodwus2111.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)y5lvsblobprodwus2179.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)zt8vsblobprodwus2176.vsblob.vsassets.io/usr/bin/dotnet dotnet build src/Compute/Compute.sln(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
This section details on the original issue you should resolve
<issue_title>Az.Compute - Create, Update, Get, Az Compute Gallery with Managed Identity</issue_title>
<issue_description>
Guidelines
The purpose of the Azure PowerShell design review is to ensure that the cmdlets follow the same pattern across the Azure modules. An early design review reduces the risk of unnecessary implementation changes caused by a cmdlet syntax design change.
Please ensure your cmdlets comply with the design guidelines outlined in the PowerShell Design Guidelines document.
Please generate cmdlets syntax using
GenerateCmdletDesignMarkdown.ps1for review if your cmdlet is generated from API spec directly by Autorest.PowerShell.If you just add parameter to existing cmdlets and parameter definition complies with guideline, the design review is not expected.
Have you read above statement?
YESService Release Details
Is this an Embargoed Preview, A Public Preview, or a General Release?
General ReleaseWhat is the expected service release date?
2026-03-15Which Powershell module are these changes being made in?
Az.ComputeContact Information
Main developer contacts (emails + github aliases)
bhbrahma@microsoft.com @D1v38om83rPM contact (email + github alias)
Tanmay.Gore@microsoft.com @tanmaygoreOther people who should attend a design review (email)
Joseph.Calev@microsoft.comHigh Level Scenarios
Describe how your feature is intended to be used by customers.
New-AzGallerycommand should allow user to specify managed identities when creating an Azure Compute GalleryUpdate-AzGalleryshould accept the updated PSObject of type Microsoft.Compute/galleries with Managed Identity information and also allow user to specify managed identities.Get-AzGallerycommand should be able to fetch updated PSObject of type Microsoft.Compute/galleries with Managed Identity informationPiping scenarios / how these cmdlets are used with existing cmdlets
Sample of end-to-end usage
Please provide comprehensive examples that don't assume additional setup. It helps the audience understand your feature.
New-AzGallery -ResourceGroupName $rgname -Name $galleryName -Location $location -SystemAssignedIdentity$uid1 = Get-AzUserAssignedIdentity -ResourceGroupName azure-rg-test -Name uai-pwsh01; New-AzGallery -ResourceGroupName $rgname -Name $galleryName -Location $location -UserAssignedIdentities $uid1$gallery = Get-AzGallery -ResourceGroupName $rgname -Name $galleryName; Update-AzGallery -inputObject gallery SystemAssignedIdentity$uid1 = Get-AzUserAssignedIdentity -ResourceGroupName azure-rg-test -Name uai-pwsh01; $gallery = Get-AzGallery -ResourceGroupName $rgname -Name $galleryName; Update-AzGallery -inputObject $gallery -UserAssginedIdentities $uid1Syntax changes
Indicate if you are requesting an edit to existing cmdlets, adding new cmdlets, or both. Then edit the corresponding section below.
Changed cmdlet
List the names of the cmdlets that are being edited.
List the new parameters for the cmdlet, the parameter types, and their allowed values if applicable.
Please describe the new business logic of the cmdlet and paramet...
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.