{Compute} az vm unmanaged-disk: Migrate command group to aaz-based implementation#32754
{Compute} az vm unmanaged-disk: Migrate command group to aaz-based implementation#32754william051200 wants to merge 3 commits intoAzure:devfrom
az vm unmanaged-disk: Migrate command group to aaz-based implementation#32754Conversation
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 the az vm unmanaged-disk command group from SDK-based (mgmt.compute) to AAZ-based implementation. The migration follows the established pattern used elsewhere in the VM module for AAZ migrations.
Changes:
- Migrated
az vm unmanaged-disk attach,detach, andlistcommands to use AAZ-based functions - Added helper function
_get_disk_lun_by_aazandDiskCreateOptionTypesenum to support the AAZ-based implementation - Removed SDK dependency from the command group declaration
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/vm/custom.py | Migrated three unmanaged disk functions to AAZ: detach, attach, and list. Added helper function _get_disk_lun_by_aaz for AAZ-based disk LUN calculation. |
| src/azure-cli/azure/cli/command_modules/vm/commands.py | Removed SDK client factory parameter from vm unmanaged-disk command group since AAZ-based implementations don't require it. |
| src/azure-cli/azure/cli/command_modules/vm/_vm_utils.py | Added DiskCreateOptionTypes enum to replace SDK-based enum in AAZ implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(vm.storage_profile.data_disks) == len(leftovers): | ||
| vm = get_vm_to_update_by_aaz(cmd, resource_group_name, vm_name) | ||
| vm = convert_show_result_to_snake_case(vm) | ||
| leftovers = [d for d in vm.get('storage_profile', {}).get('data_disks', []) if d.get('name').lower() != disk_name.lower()] |
There was a problem hiding this comment.
The expression d.get('name').lower() will raise an AttributeError if the 'name' key is missing from the disk dictionary, as d.get('name') would return None. Change to d.get('name', '').lower() to provide a default empty string when the key is missing.
| leftovers = [d for d in vm.get('storage_profile', {}).get('data_disks', []) if d.get('name').lower() != disk_name.lower()] | |
| leftovers = [d for d in vm.get('storage_profile', {}).get('data_disks', []) if d.get('name', '').lower() != disk_name.lower()] |
| if vm.get('storage_profile', {}).get('data_disks'): | ||
| vm['storage_profile']['data_disks'] = leftovers |
There was a problem hiding this comment.
The conditional check on line 2456 is unnecessary and differs from the original implementation. Since the error check on line 2453 confirms that data_disks exist and contain the disk to remove, the assignment should happen unconditionally. Change to: vm['storage_profile']['data_disks'] = leftovers without the if condition, matching the original logic where vm.storage_profile.data_disks = leftovers was always executed.
| if vm.get('storage_profile', {}).get('data_disks'): | |
| vm['storage_profile']['data_disks'] = leftovers | |
| vm['storage_profile']['data_disks'] = leftovers |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| self.assertIsNotNone(converted[0].get('managedDisk')) | ||
| self.assertIsNone(converted[0].get('vhd')) |
There was a problem hiding this comment.
May I ask why we need to change these verified functions?
There was a problem hiding this comment.
Reason as in result return from aaz will not include managedDisk and vhd when there is no value.
There was a problem hiding this comment.
does the previous logic return converted[0]['managedDisk'] as True and converted[0]['vhd'] as False?
There was a problem hiding this comment.
No, it is actually returning in either "None" or "{}" object, there is no True or False
4 participants
Related command
az vm unmanaged-disk attachaz vm unmanaged-disk detachaz vm unmanaged-disk listDescription
Migration from mgmt.compute to aaz-based
Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis 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.