{Compute} az vm: Migrate commands to aaz-based implementation#32718
Open
william051200 wants to merge 25 commits intoAzure:devfrom
Open
{Compute} az vm: Migrate commands to aaz-based implementation#32718william051200 wants to merge 25 commits intoAzure:devfrom
az vm: Migrate commands to aaz-based implementation#32718william051200 wants to merge 25 commits intoAzure:devfrom
Conversation
william051200
requested review from
jsntcy,
wangzelin007,
yanzhudd and
zhoxing-ms
as code owners
❌AzureCLI-FullTest
|
️✔️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 pull request migrates core az vm commands from the mgmt.compute SDK to the AAZ (Azure CLI Auto-generated) implementation. This is part of a broader effort to modernize the Azure CLI codebase by adopting auto-generated command implementations.
Changes:
- Migrates
az vm create,az vm list,az vm update, andaz vm showcommands to AAZ-based implementation - Adds AAZ-based SSH key commands (
az sshkey create,show,list,update,delete) - Updates validators and utilities to work with dictionary-based data structures instead of SDK objects
- Removes deprecated API version checks that are no longer needed with AAZ
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| custom.py | Updates VM creation and management functions to use AAZ-based get_vm_by_aaz; migrates VM extension creation to AAZ |
| commands.py | Reorganizes command registration to separate AAZ-based commands from SDK-based commands |
| _update.py | Adds new AAZ-based sshkey update command implementation |
| _show.py | Adds new AAZ-based sshkey show command implementation |
| _list.py | Adds new AAZ-based sshkey list command implementation |
| _delete.py | Adds new AAZ-based sshkey delete command implementation |
| _create.py | Adds new AAZ-based sshkey create command implementation |
| init.py | Exports new sshkey command classes |
| __cmd_group.py | Defines sshkey command group |
| _vm_utils.py | Updates disk caching function to work with AAZ dictionary structures; removes duplicate function |
| _validators.py | Migrates validators to use AAZ commands; updates data structure access patterns from attributes to dictionary keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
18
to
20
| """Update a new SSH public key resource. | ||
|
|
||
| :example: Create a new SSH public key resource. |
There was a problem hiding this comment.
The docstring and example are inconsistent. The docstring says "Update a new SSH public key resource" but should say "Update an existing SSH public key resource" since this is an update command, not a create command. Similarly, the example description says "Create a new SSH public key resource" but should say "Update an SSH public key resource".
Suggested change
| """Update a new SSH public key resource. | |
| :example: Create a new SSH public key resource. | |
| """Update an existing SSH public key resource. | |
| :example: Update an SSH public key resource. |
Comment on lines
616
to
617
| raise ArgumentUsageError("The --os-type is not the correct os type of this shared gallery image, " | ||
| "the os type of this image should be {}".format(shared_gallery_image_info.os_type)) |
There was a problem hiding this comment.
The error message on line 617 references 'shared_gallery_image_info.os_type' as an attribute, but after migrating to AAZ, the data structure is a dictionary. This should be changed to use dictionary access: shared_gallery_image_info.get('osType') or shared_gallery_image_info['osType'].
Comment on lines
639
to
640
| "The --os-type is not the correct os type of this community gallery image, " | ||
| "the os type of this image should be {}".format(community_gallery_image_info.os_type)) |
There was a problem hiding this comment.
The error message on line 640 references 'community_gallery_image_info.os_type' as an attribute, but after migrating to AAZ, the data structure is a dictionary. This should be changed to use dictionary access: community_gallery_image_info.get('osType') or community_gallery_image_info['osType'].
| 'location': namespace.location | ||
| }) | ||
|
|
||
| size_info = next((s for s in sizes if s.name.lower() == size), None) |
There was a problem hiding this comment.
After migrating to AAZ, the 'sizes' variable returned from ListSizes is a list of dictionaries, not objects. The comparison on line 974 tries to access 's.name' as an attribute, but it should be 's.get("name")' or 's["name"]' to access the dictionary key.
Suggested change
| size_info = next((s for s in sizes if s.name.lower() == size), None) | |
| size_info = next((s for s in sizes if s.get('name', '').lower() == size), None) |
Comment on lines
1279
to
1284
| if vm.get('storageProfile', {}).get('osDisk', {}).get('osType', '') == 'Linux': | ||
| publisher = 'Microsoft.Azure.Security.LinuxAttestation' | ||
| if vm.storage_profile.os_disk.os_type == 'Windows': | ||
| if vm.get('storageProfile', {}).get('osDisk', {}).get('osType', '') == 'Windows': | ||
| publisher = 'Microsoft.Azure.Security.WindowsAttestation' | ||
| version = _normalize_extension_version(cmd.cli_ctx, publisher, 'GuestAttestation', None, vm.location) | ||
| VirtualMachineExtension = cmd.get_models('VirtualMachineExtension') | ||
| ext = VirtualMachineExtension(location=vm.location, | ||
| publisher=publisher, | ||
| type_properties_type='GuestAttestation', | ||
| protected_settings=None, | ||
| type_handler_version=version, | ||
| settings=None, | ||
| auto_upgrade_minor_version=True, | ||
| enable_automatic_upgrade=not disable_integrity_monitoring_autoupgrade) | ||
|
|
||
| version = _normalize_extension_version(cmd.cli_ctx, publisher, 'GuestAttestation', None, vm['location']) |
There was a problem hiding this comment.
The variable 'publisher' is used on line 1284 and 1294, but it may not be initialized if the OS type is neither 'Linux' nor 'Windows'. Lines 1279 and 1281 use independent 'if' statements rather than 'if-elif', which means if the osType doesn't match either condition, 'publisher' will be undefined when used later, causing a NameError. Consider using 'if-elif-else' structure or initializing 'publisher' to a default value before these conditions.
| 'resource_group': namespace.resource_group_name, | ||
|
|
||
| } | ||
| ImageShow(cli_ctx=cmd.cli_ctx, command_args=command_args) |
There was a problem hiding this comment.
The ImageShow command is being invoked but its result is not being stored or checked. The function should call the command with command_args and store the result to verify if the image exists. The current code just instantiates the class but doesn't execute it.
Suggested change
| ImageShow(cli_ctx=cmd.cli_ctx, command_args=command_args) | |
| image_show = ImageShow(cli_ctx=cmd.cli_ctx) | |
| result = image_show(command_args) | |
| if not result: | |
| raise HttpResponseError("Image '{}' not found.".format(namespace.image)) |
|
|
||
| namespace.os_type = image_info.get('storageProfile', {}).get('osDisk', {}).get('osType') | ||
| image_data_disks = image_info.get('storageProfile', {}).get('dataDisks', []) | ||
| image_data_disks = [{'lun': disk.lun} for disk in image_data_disks] |
There was a problem hiding this comment.
The image_data_disks list comprehension is trying to access disk.lun as an attribute, but after the migration to AAZ, image_data_disks is a list of dictionaries, not objects. This should be changed to use dictionary access: [{'lun': disk['lun']} for disk in image_data_disks] or [{'lun': disk.get('lun')} for disk in image_data_disks].
Suggested change
| image_data_disks = [{'lun': disk.lun} for disk in image_data_disks] | |
| image_data_disks = [{'lun': disk["lun"]} for disk in image_data_disks] |
Comment on lines
346
to
347
| image_version = _get_latest_image_version(cmd.cli_ctx, namespace.location, namespace.os_publisher, | ||
| namespace.os_offer, namespace.os_sku) |
There was a problem hiding this comment.
This function calls _get_latest_image_version (the old non-AAZ version) instead of _get_latest_image_version_by_aaz (the AAZ version). For consistency with the migration to AAZ-based implementation, this should be updated to use _get_latest_image_version_by_aaz, which was already imported at line 27.
Suggested change
| image_version = _get_latest_image_version(cmd.cli_ctx, namespace.location, namespace.os_publisher, | |
| namespace.os_offer, namespace.os_sku) | |
| image_version = _get_latest_image_version_by_aaz(cmd.cli_ctx, namespace.location, namespace.os_publisher, | |
| namespace.os_offer, namespace.os_sku) |
william051200
force-pushed
the
vm-migration
branch
2 times, most recently
from
c09ee94 to
9bd40ff
Compare
william051200
force-pushed
the
vm-migration
branch
from
a03d9c2 to
4cbf8b4
Compare
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
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 vm createaz vm listaz vm updateaz vm showDescription
Migration from mgmt.compute to aaz-based
aaz Azure/aaz#935
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.