[Compute] Update command az sig create, az sig show and add command group az sig identity to manage the service identity of a Shared Image Gallery (SIG)#33137
Open
william051200 wants to merge 20 commits intoAzure:devfrom
Conversation
william051200
requested review from
NoriZC,
teresaritorto and
yanzhudd
as code owners
️✔️AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| sig create | cmd sig create added parameter assign_identity |
||
| sig create | cmd sig create added parameter identity_role |
||
| sig create | cmd sig create added parameter identity_scope |
||
| sig create | cmd sig create update parameter description: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter description: removed property type=string |
||
| sig create | cmd sig create update parameter eula: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter eula: removed property type=string |
||
| sig create | cmd sig create update parameter gallery_name: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter gallery_name: removed property type=string |
||
| sig create | cmd sig create update parameter location: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter location: updated property type from string to custom_type |
||
| sig create | cmd sig create update parameter no_wait: removed property aaz_type=bool |
||
| sig create | cmd sig create update parameter no_wait: removed property choices=['0', '1', 'f', 'false', 'n', 'no', 't', 'true', 'y', 'yes'] |
||
| sig create | cmd sig create update parameter no_wait: removed property nargs=? |
||
| sig create | cmd sig create update parameter no_wait: removed property type=bool |
||
| sig create | cmd sig create update parameter permissions: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter permissions: removed property type=string |
||
| sig create | cmd sig create update parameter public_name_prefix: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter public_name_prefix: removed property type=string |
||
| sig create | cmd sig create update parameter publisher_contact: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter publisher_contact: removed property type=string |
||
| sig create | cmd sig create update parameter publisher_uri: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter publisher_uri: removed property type=string |
||
| sig create | cmd sig create update parameter resource_group: removed property aaz_type=string |
||
| sig create | cmd sig create update parameter resource_group: removed property type=string |
||
| sig create | cmd sig create update parameter resource_group: updated property name from resource_group to resource_group_name |
||
| sig create | cmd sig create update parameter soft_delete: removed property aaz_type=bool |
||
| sig create | cmd sig create update parameter soft_delete: removed property type=bool |
||
| sig create | cmd sig create update parameter soft_delete: updated property choices from ['0', '1', 'f', 'false', 'n', 'no', 't', 'true', 'y', 'yes'] to ['false', 'true'] |
||
| sig create | cmd sig create update parameter tags: removed property aaz_type=AAZDictArg |
||
| sig create | cmd sig create update parameter tags: removed property type=Dict<String,String> |
||
| sig create | cmd sig create update parameter tags: updated property nargs from + to * |
||
| sig identity | sub group sig identity added |
Collaborator
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
requested review from
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
the
Compute
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the sig (Shared Image Gallery) commands in the VM module to support Managed Service Identity (system/user-assigned) on SIG resources, and introduces a new az sig identity command group for identity management (assign/remove/show), aligning with the feature request in issue #32814.
Changes:
- Extend
az sig createto optionally configure managed identity (and optionally create an RBAC role assignment via--scope/--role). - Add
az sig identity assign|remove|showcommands backed by new AAZ identity subresource plumbing and custom wrappers. - Add/extend scenario tests validating SIG MSI behaviors (system-assigned + user-assigned).
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_vm_commands.py | Adds scenario tests for SIG identity assign/remove/show and create/show output validation. |
| src/azure-cli/azure/cli/command_modules/vm/operations/sig.py | Adds SIG identity remove/show operation customizations on top of generated AAZ commands. |
| src/azure-cli/azure/cli/command_modules/vm/custom.py | Introduces custom implementations for sig create and sig identity assign/remove integration and output shaping. |
| src/azure-cli/azure/cli/command_modules/vm/commands.py | Wires sig create to the new custom handler and registers the new sig identity command group. |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/identity/_assign.py | Generated AAZ command for identity assignment. |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/identity/_remove.py | Generated AAZ command for identity removal. |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/identity/_show.py | Generated AAZ command for showing identity (docstring/examples currently inaccurate). |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/identity/_wait.py | Generated AAZ wait command (currently not imported/registered). |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/identity/init.py | Exposes generated identity commands (but does not expose _wait). |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/identity/__cmd_group.py | Generated command group registration for sig identity. |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/_create.py | Updates SIG create to new API version and adds identity schema handling. |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/_show.py | Updates SIG show to new API version and adds identity schema handling. |
| src/azure-cli/azure/cli/command_modules/vm/aaz/latest/sig/_wait.py | Updates SIG wait command to new API version and identity/systemData schema. |
| src/azure-cli/azure/cli/command_modules/vm/_vm_utils.py | Expands principal id discovery to support identity operations returning flattened identity payloads. |
| src/azure-cli/azure/cli/command_modules/vm/_validators.py | Adds validators for SIG identity arguments and RBAC scope/role constraints. |
| src/azure-cli/azure/cli/command_modules/vm/_params.py | Adds CLI parameters for SIG MSI and sig identity commands. |
| src/azure-cli/azure/cli/command_modules/vm/_help.py | Adds help entries for sig create and sig identity assign/remove. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+6713
to
+6716
| if assign_identity is None: | ||
| create_sig = SigCreate(cli_ctx=cmd.cli_ctx)(command_args=command_args) | ||
| LongRunningOperation(cmd.cli_ctx)(create_sig) | ||
| return create_sig.result() |
There was a problem hiding this comment.
create_sig eagerly waits on the AAZ LRO via LongRunningOperation and then calls .result() again. This defeats the CLI's standard --no-wait behavior (the command will still block) and does redundant polling. Prefer returning the poller (or using sdk_no_wait(no_wait, ...)) and let the command invoker handle polling/--no-wait, then post-process the final result only when no_wait is false.
| return sig | ||
|
|
||
|
|
||
| def _assign_identity(resource, identity, external_identities=[], enable_local_identity=None, command_args={}): |
There was a problem hiding this comment.
_assign_identity uses mutable default arguments (external_identities=[], command_args={}), which can leak state across calls and cause hard-to-debug behavior. Use None defaults and initialize new lists/dicts inside the function.
Suggested change
| def _assign_identity(resource, identity, external_identities=[], enable_local_identity=None, command_args={}): | |
| def _assign_identity(resource, identity, external_identities=None, enable_local_identity=None, command_args=None): | |
| if external_identities is None: | |
| external_identities = [] | |
| if command_args is None: | |
| command_args = {} |
|
|
||
| if existing_identity['type'] == IdentityType.NONE.value \ | ||
| or existing_identity['type'] == IdentityType.SYSTEM_ASSIGNED.value: | ||
| existing_identity.pop('userAssignedIdentities') |
There was a problem hiding this comment.
In pre_instance_update, userAssignedIdentities can be popped twice: when removing the last user-assigned identity, it's popped inside the len(emsis_to_retain) < 1 block and then popped again later when type becomes None/SystemAssigned. This will raise KeyError in that path. Use pop('userAssignedIdentities', None) (or guard with if 'userAssignedIdentities' in ...) for the later cleanup.
Suggested change
| existing_identity.pop('userAssignedIdentities') | |
| existing_identity.pop('userAssignedIdentities', None) |
Comment on lines
+1368
to
+1372
| with self.argument_context('sig identity remove') as c: | ||
| c.argument('identities', nargs='*', help="Space-separated identities to assign. Use '{0}' to refer to the system assigned identity. Default: '{0}'".format(MSI_LOCAL_ID)) | ||
|
|
||
| with self.argument_context('sig identity assign') as c: | ||
| c.argument('assign_identity', options_list=['--identities'], nargs='*', help="Space-separated identities to assign. Use '{0}' to refer to the system assigned identity. Default: '{0}'".format(MSI_LOCAL_ID)) |
There was a problem hiding this comment.
Help text for sig identity remove --identities says "identities to assign", but this command removes identities. This is user-facing and confusing; update the help string to say "remove" (and keep it consistent with sig identity assign).
Comment on lines
+591
to
+606
| helps['sig identity assign'] = """ | ||
| type: command | ||
| short-summary: Assign the user or system managed identities. | ||
| """ | ||
|
|
||
| helps['sig identity remove'] = """ | ||
| type: command | ||
| short-summary: Remove the user or system managed identities. | ||
| examples: | ||
| - name: Remove the system assigned identity. | ||
| text: | | ||
| az sig identity remove --resource-group myResourceGroup --gallery-name myGalleryName | ||
| - name: Remove a user assigned identity. | ||
| text: | | ||
| az sig identity remove --resource-group myResourceGroup --gallery-name myGalleryName --identities readerId | ||
| """ |
There was a problem hiding this comment.
The sig identity command group includes a show command (registered in commands.py), but there is no corresponding help entry here. Add helps['sig identity show'] (and examples if appropriate) to keep help coverage consistent with assign/remove.
| class Show(AAZCommand): | ||
| """Show the details of managed identities. | ||
|
|
||
| :example: Get a community gallery. |
There was a problem hiding this comment.
The command docstring/examples are incorrect for sig identity show (it says "Get a community gallery"). This text is user-facing when help is generated from docstrings; update it to describe showing the SIG managed identity instead.
Suggested change
| :example: Get a community gallery. | |
| :example: Show the managed identity of a gallery. |
| from .__cmd_group import * | ||
| from ._assign import * | ||
| from ._remove import * | ||
| from ._show import * |
There was a problem hiding this comment.
sig identity _wait.py is added, but sig.identity.__init__ doesn't import/export it, so the decorator never runs and az sig identity wait won't be registered. Either add _wait to __init__.py (if the command should exist) or remove the unused generated file to avoid dead code.
Suggested change
| from ._show import * | |
| from ._show import * | |
| from ._wait import * |
This file contains hidden or 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
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
az sig createaz sig showaz sig identity assignaz sig identity removeaz sig identity showDescription
Resolve #32814
aaz Azure/aaz#977
Testing Guide
Test cases:
vm\tests\latest\test_vm_commands.py: test_sig_msi
vm\tests\latest\test_vm_commands.py: test_sig_explicit_msi
History Notes
[Compute]
az sig create: Add new argument group "Managed Service Identity" to configure the service identity of a Shared Image Gallery (SIG)[Compute]
az sig show: Update command to display the managed service identity of a Shared Image Gallery (SIG)[Compute]
az sig identity: Add new command group to manage the service identity of a Shared Image Gallery (SIG)This 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.