{Compute} az vm secret: Migrate command group to aaz-based implementation
#32626
Conversation
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 PR migrates the az vm secret command group (add, list, remove) from the legacy mgmt.compute SDK to the new aaz-based implementation. The migration updates the internal implementation while maintaining the same external API surface.
Changes:
- Added aaz-based helper functions:
get_vm_by_aaz(),set_vm_by_aaz(), and updatedget_vm_to_update_by_aaz() - Migrated three secret management commands to use aaz dictionaries instead of SDK models
- Updated property access patterns from snake_case to camelCase to match AAZ naming conventions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'certificate_store': certificate_store, | ||
| 'certificate_url': certificate |
There was a problem hiding this comment.
The dictionary key should be 'certificateStore' (camelCase) to match the AAZ naming convention used elsewhere in this migration. The current snake_case 'certificate_store' is inconsistent with the camelCase pattern used in other parts of the code (e.g., 'certificateUrl', 'sourceVault', 'vaultCertificates').
| 'certificate_store': certificate_store, | |
| 'certificate_url': certificate | |
| 'certificateStore': certificate_store, | |
| 'certificateUrl': certificate |
| 'certificate_store': certificate_store, | ||
| 'certificate_url': certificate |
There was a problem hiding this comment.
The dictionary key should be 'certificateUrl' (camelCase) to match the AAZ naming convention used elsewhere in this migration. The current snake_case 'certificate_url' is inconsistent with the camelCase pattern used in other parts of the code (e.g., 'certificateStore' on line 3243, 'sourceVault', 'vaultCertificates').
| 'certificate_store': certificate_store, | |
| 'certificate_url': certificate | |
| 'certificateStore': certificate_store, | |
| 'certificateUrl': certificate |
| }, | ||
| 'vaultCertificates': [vault_cert] | ||
| } | ||
| vm.get('osProfile', {}).get('secrets', []).append(vault_secret_group) |
There was a problem hiding this comment.
Bug: When 'secrets' key doesn't exist in osProfile, vm.get('osProfile', {}).get('secrets', []) returns a new empty list, and appending to that list won't modify the vm dictionary. This line should either initialize the secrets list if it doesn't exist before appending, or use a pattern like: if 'secrets' not in vm['osProfile']: vm['osProfile']['secrets'] = []; vm['osProfile']['secrets'].append(vault_secret_group)
| if keyvault: | ||
| keyvault = keyvault.lower() | ||
| keyvault_matched = [x for x in to_keep if x.source_vault and x.source_vault.id.lower() == keyvault] | ||
| keyvault_matched = [x for x in to_keep if x.get("sourceVault", {}).get('id', '').lower() == keyvault] |
There was a problem hiding this comment.
Inconsistent quote usage: The key "sourceVault" uses double quotes while 'id' uses single quotes. For consistency with the rest of the codebase, use either all double quotes or all single quotes within the same dictionary access chain.
| keyvault_matched = [x for x in to_keep if x.get("sourceVault", {}).get('id', '').lower() == keyvault] | |
| keyvault_matched = [x for x in to_keep if x.get('sourceVault', {}).get('id', '').lower() == keyvault] |
william051200
force-pushed
the
vm-secret-migration
branch
from
e83c2db to
4663dad
Compare
yanzhudd
changed the title
az vm secret: Migrate command group to aaz-based implementation
| if x.get('sourceVault', {}).get('id', '').lower() == keyvault.lower()), None) | ||
| if vault_secret_group: | ||
| vault_secret_group.vault_certificates.append(vault_cert) | ||
| vault_secret_group.get('vaultCertificates', []).append(vault_cert) |
There was a problem hiding this comment.
Please note that the get(key, default_value) function returns the default value when the key does not exist in the dictionary. However, this default value is not automatically added to the dictionary.
| vault_secret_group.get('vaultCertificates', []).append(vault_cert) | |
| certs = vault_secret_group.get('vaultCertificates', []) | |
| certs.append(vault_cert) | |
| vault_secret_group['vaultCertificates'] = certs |
| certificate = cert_info.secret_id | ||
|
|
||
| if not _is_linux_os(vm): | ||
| if not _is_linux_os_by_aaz(vm): |
There was a problem hiding this comment.
the _is_linux_os_by_aaz function is based on the snake case, so we might need to convert the vm to snake case first
4 participants
Related command
az vm secret addaz vm secret listaz vm secret removeDescription
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.