{Compute} az vm: Migrate commands to aaz-based implementation#32718
{Compute} az vm: Migrate commands to aaz-based implementation#32718william051200 wants to merge 33 commits intoAzure:devfrom
az vm: Migrate commands to aaz-based implementation#32718Conversation
william051200
requested review from
jsntcy,
wangzelin007,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
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
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.
| """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".
| """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. |
| 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'].
| "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.
| 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) |
| 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.
| 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].
| image_data_disks = [{'lun': disk.lun} for disk in image_data_disks] | |
| image_data_disks = [{'lun': disk["lun"]} for disk in image_data_disks] |
| 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.
| 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
4 participants
Related command
az vm createaz vm listaz vm updateaz vm showaz vm image listaz vm image 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.