{Compute} az snapshot create: Migrate command to aaz-based implementation
#32584
+58
−10
Conversation
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
william051200
requested review from
jsntcy,
wangzelin007,
yanzhudd and
zhoxing-ms
as code owners
❌AzureCLI-FullTest
|
|
Hi @william051200, |
️✔️AzureCLI-BreakingChangeTest
|
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
added
the
Compute
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the az snapshot create command from mgmt.compute SDK to AAZ-based implementation. While the migration represents progress toward modernizing the codebase, the approach taken for resource ID resolution introduces some concerns.
Key Changes:
- Removed unused import of
get_subscription_idfrom client_factory - Replaced resource_id() construction with AAZ Show commands for disk_encryption_set and disk_access resolution
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
768
to
772
| from .aaz.latest.disk_encryption_set import Show as DiskEncryptionSetShow | ||
| disk_encryption_set = DiskEncryptionSetShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_encryption_set_name': disk_encryption_set | ||
| })['id'] |
There was a problem hiding this comment.
The new AAZ-based implementation lacks error handling when the disk_encryption_set resource doesn't exist. If the Show command fails (e.g., resource not found), it will raise an exception that may not provide a clear, user-friendly error message in the context of snapshot creation. Consider adding try-except block with HttpResponseError to provide a more contextual error message, similar to the pattern used in _validators.py line 1636-1650.
Comment on lines
775
to
779
| from .aaz.latest.disk_access import Show as DiskAccessShow | ||
| disk_access = DiskAccessShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_access_name': disk_access | ||
| })['id'] |
There was a problem hiding this comment.
The new AAZ-based implementation lacks error handling when the disk_access resource doesn't exist. If the Show command fails (e.g., resource not found), it will raise an exception that may not provide a clear, user-friendly error message in the context of snapshot creation. Consider adding try-except block with HttpResponseError to provide a more contextual error message, similar to the pattern used in _validators.py line 1636-1650.
Comment on lines
768
to
772
| from .aaz.latest.disk_encryption_set import Show as DiskEncryptionSetShow | ||
| disk_encryption_set = DiskEncryptionSetShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_encryption_set_name': disk_encryption_set | ||
| })['id'] |
There was a problem hiding this comment.
This migration introduces an unnecessary API call to retrieve the full resource just to get its ID. The original implementation using resource_id() was more efficient as it simply constructed the resource ID string without making an API call. This change impacts performance, especially when creating multiple snapshots or in scenarios with rate limiting. Consider using resource_id() from azure.mgmt.core.tools to construct the ID directly, which is consistent with how create_managed_disk handles the same scenario (lines 552-555).
Comment on lines
775
to
779
| from .aaz.latest.disk_access import Show as DiskAccessShow | ||
| disk_access = DiskAccessShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_access_name': disk_access | ||
| })['id'] |
There was a problem hiding this comment.
This migration introduces an unnecessary API call to retrieve the full resource just to get its ID. The original implementation using resource_id() was more efficient as it simply constructed the resource ID string without making an API call. This change impacts performance, especially when creating multiple snapshots or in scenarios with rate limiting. Consider using resource_id() from azure.mgmt.core.tools to construct the ID directly, which is consistent with how create_managed_disk handles the same scenario (lines 557-560).
Comment on lines
768
to
779
| from .aaz.latest.disk_encryption_set import Show as DiskEncryptionSetShow | ||
| disk_encryption_set = DiskEncryptionSetShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_encryption_set_name': disk_encryption_set | ||
| })['id'] | ||
|
|
||
| if disk_access is not None and not is_valid_resource_id(disk_access): | ||
| disk_access = resource_id( | ||
| subscription=get_subscription_id(cmd.cli_ctx), resource_group=resource_group_name, | ||
| namespace='Microsoft.Compute', type='diskAccesses', name=disk_access) | ||
| from .aaz.latest.disk_access import Show as DiskAccessShow | ||
| disk_access = DiskAccessShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_access_name': disk_access | ||
| })['id'] |
There was a problem hiding this comment.
Inconsistent migration approach within the same codebase. The create_managed_disk function (lines 552-560) still uses the resource_id() approach for disk_encryption_set and disk_access, while this PR migrates create_snapshot to use AAZ Show commands. This inconsistency makes the code harder to maintain and creates different behaviors for similar operations. Both functions should use the same pattern.
yanzhudd
changed the title
az snapshot create: Migrate command to aaz-based implementation
yanzhudd
reviewed
Dec 31, 2025
| 'format, please refer to the help sample'.format(gallery_image_reference)) | ||
|
|
||
|
|
||
| def _validate_gallery_image_reference_by_aaz(namespace): |
Contributor
There was a problem hiding this comment.
May I ask why we need to define a separate function instead of reusing _validate_gallery_image_reference function?
yanzhudd
reviewed
Dec 31, 2025
Comment on lines
-769
to
-771
| disk_encryption_set = resource_id( | ||
| subscription=get_subscription_id(cmd.cli_ctx), resource_group=resource_group_name, | ||
| namespace='Microsoft.Compute', type='diskEncryptionSets', name=disk_encryption_set) |
Contributor
There was a problem hiding this comment.
this part of logic is used to build a resource id. may I ask why it needs to be removed?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 snapshot createDescription
Migration from mgmt.compute to aaz-based
Testing Guide
History Notes
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.