{Compute} disk command migration #32588
Closed
+99
−23
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 VM disk commands from the legacy mgmt.compute SDK to the newer AAZ (Azure CLI Auto-generation)-based approach. The migration affects the az disk create and az disk update commands.
Key Changes
- Replaced
resource_id()utility calls with AAZShowcommand invocations fordisk-encryption-setanddisk-accessresources to retrieve full resource IDs - Removed the
compute_disk_sdkparameter from the disk command group registration - Added a new
_validate_gallery_image_reference_by_aazvalidator function to replace the SDK-based version
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/vm/operations/disk.py |
Updated DiskUpdate class to use AAZ Show commands (DiskEncryptionSetShow, DiskAccessShow) instead of resource_id utility for resolving resource names to IDs |
src/azure-cli/azure/cli/command_modules/vm/custom.py |
Updated create_managed_disk function to use AAZ Show commands for disk_encryption_set, disk_access, and secure_vm_disk_encryption_set resolution |
src/azure-cli/azure/cli/command_modules/vm/commands.py |
Removed compute_disk_sdk parameter from disk command group, completing migration to AAZ-based commands |
src/azure-cli/azure/cli/command_modules/vm/_validators.py |
Added new _validate_gallery_image_reference_by_aaz function and commented out API version check in _validate_upload_type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2059
to
2063
| # if not cmd.supported_api_version(min_api='2021-08-01', operation_group='disks'): | ||
| # raise ArgumentUsageError( | ||
| # "'UploadWithSecurityData' is not supported in the current profile. " | ||
| # "Please upgrade your profile with 'az cloud set --profile newerProfile' and try again") | ||
|
|
There was a problem hiding this comment.
Commented-out code should be removed. This API version check was commented out during the migration to AAZ, but commented code should not be left in production. If the check is no longer needed because AAZ handles API version compatibility automatically, remove these lines entirely. If the check is still needed, it should be uncommented and properly implemented for AAZ commands.
Suggested change
| # if not cmd.supported_api_version(min_api='2021-08-01', operation_group='disks'): | |
| # raise ArgumentUsageError( | |
| # "'UploadWithSecurityData' is not supported in the current profile. " | |
| # "Please upgrade your profile with 'az cloud set --profile newerProfile' and try again") |
Comment on lines
50
to
58
| disk_encryption_set = DiskEncryptionSetShow(cli_ctx=self.cli_ctx)(command_args={ | ||
| 'resource_group': args.resource_group, | ||
| 'disk_encryption_set_name': disk_encryption_set | ||
| }) | ||
|
|
||
| if disk_encryption_set: | ||
| disk_encryption_set = disk_encryption_set['id'] | ||
| else: | ||
| disk_encryption_set = None |
There was a problem hiding this comment.
Missing error handling for DiskEncryptionSetShow. AAZ Show commands raise HttpResponseError when the resource is not found, but this code doesn't catch that exception. The current code will fail with an unhandled exception if the disk encryption set doesn't exist. You should wrap this call in a try-except block to catch HttpResponseError, similar to the pattern used in get_network_lb in _validators.py (lines 1720-1726).
Comment on lines
69
to
77
| disk_access = DiskAccessShow(cli_ctx=self.cli_ctx)(command_args={ | ||
| 'resource_group': args.resource_group, | ||
| 'disk_access_name': disk_access | ||
| }) | ||
|
|
||
| if disk_access: | ||
| disk_access = disk_access['id'] | ||
| else: | ||
| disk_access = None |
There was a problem hiding this comment.
Missing error handling for DiskAccessShow. AAZ Show commands raise HttpResponseError when the resource is not found, but this code doesn't catch that exception. The current code will fail with an unhandled exception if the disk access doesn't exist. You should wrap this call in a try-except block to catch HttpResponseError, similar to the pattern used in get_network_lb in _validators.py (lines 1720-1726).
| disk_access = disk_access['id'] | ||
| else: | ||
| disk_access = None | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected on this line. Please remove it.
Suggested change
Comment on lines
554
to
562
| disk_encryption_set = DiskEncryptionSetShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_encryption_set_name': disk_encryption_set | ||
| }) | ||
|
|
||
| if disk_encryption_set: | ||
| disk_encryption_set = disk_encryption_set['id'] | ||
| else: | ||
| disk_encryption_set = None |
There was a problem hiding this comment.
Missing error handling for DiskEncryptionSetShow. AAZ Show commands raise HttpResponseError when the resource is not found, but this code doesn't catch that exception. The current code will fail with an unhandled exception if the disk encryption set doesn't exist. You should wrap this call in a try-except block to catch HttpResponseError, similar to the pattern used in get_network_lb in _validators.py (lines 1720-1726).
Comment on lines
566
to
574
| disk_access = DiskAccessShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_access_name': disk_access | ||
| }) | ||
|
|
||
| if disk_access: | ||
| disk_access = disk_access['id'] | ||
| else: | ||
| disk_access = None |
There was a problem hiding this comment.
Missing error handling for DiskAccessShow. AAZ Show commands raise HttpResponseError when the resource is not found, but this code doesn't catch that exception. The current code will fail with an unhandled exception if the disk access doesn't exist. You should wrap this call in a try-except block to catch HttpResponseError, similar to the pattern used in get_network_lb in _validators.py (lines 1720-1726).
Comment on lines
578
to
586
| secure_vm_disk_encryption_set = DiskEncryptionSetShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| 'resource_group': resource_group_name, | ||
| 'disk_encryption_set_name': secure_vm_disk_encryption_set | ||
| }) | ||
|
|
||
| if secure_vm_disk_encryption_set: | ||
| secure_vm_disk_encryption_set = secure_vm_disk_encryption_set['id'] | ||
| else: | ||
| secure_vm_disk_encryption_set = None |
There was a problem hiding this comment.
Missing error handling for DiskEncryptionSetShow. AAZ Show commands raise HttpResponseError when the resource is not found, but this code doesn't catch that exception. The current code will fail with an unhandled exception if the secure VM disk encryption set doesn't exist. You should wrap this call in a try-except block to catch HttpResponseError, similar to the pattern used in get_network_lb in _validators.py (lines 1720-1726).
william051200
changed the title
william051200
changed the title
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 disk createaz disk updateDescription
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.