{Servicefabric} az sf: Migrate commands calling Compute module to aaz-based implementation#32960
Open
william051200 wants to merge 3 commits intoAzure:devfrom
Open
{Servicefabric} az sf: Migrate commands calling Compute module to aaz-based implementation#32960william051200 wants to merge 3 commits intoAzure:devfrom
william051200 wants to merge 3 commits intoAzure:devfrom
Conversation
william051200
requested review from
jsntcy,
wangzelin007 and
yanzhudd
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
requested review from
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
Service Fabric
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates several Service Fabric (az sf) commands from using the compute SDK client (compute_client_factory / mgmt.compute) to AAZ-based implementations (VMSSCreate, VMSSList from vm.operations.vmss). It also adds a new VMSSList class in the VM operations module to handle flatten conflicts for list operations.
Changes:
- Replaced SDK-based VMSS operations (list, create_or_update) with AAZ command invocations (
VMSSCreate,VMSSList) across multiple SF cluster management functions. - Added
VMSSListclass invm/operations/vmss.pyto resolve extension type flatten conflicts, mirroring the pattern used byVMSSShowandVMSSPatch. - Converted SDK model object access (attribute-based) to dict-based access throughout the SF custom module.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/vm/operations/vmss.py |
Added VMSSList class with flatten conflict resolution for both per-RG and all-subscription list operations. |
src/azure-cli/azure/cli/command_modules/servicefabric/custom.py |
Replaced compute SDK calls with AAZ VMSSCreate/VMSSList; converted model objects to dicts; updated property access patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| fabric_ext_ref['settings']['durabilityLevel'] = durability_level | ||
| fabric_ext_ref['settings']['enableParallelJobs'] = True | ||
| vmss['resource_group'] = resource_group_name | ||
| vmss['vm_scale_set_name'] = vmss.name |
There was a problem hiding this comment.
vmss.name uses attribute access on a dict, which will raise AttributeError. Should be vmss['name'].
Suggested change
| vmss['vm_scale_set_name'] = vmss.name | |
| vmss['vm_scale_set_name'] = vmss['name'] |
Comment on lines
+697
to
+701
| format(reliability_level, instance_target, vmss.sku.capacity)) | ||
| vmss.sku.capacity = instance_target | ||
| vmss_poll = compute_client.virtual_machine_scale_sets.begin_create_or_update( | ||
| resource_group_name, vmss.name, vmss) | ||
| vmss['sku']['capacity'] = instance_target | ||
| from ..vm.operations.vmss import VMSSCreate | ||
| vmss['resource_group'] = resource_group_name | ||
| vmss['vm_scale_set_name'] = vmss.name |
There was a problem hiding this comment.
Two issues on this line: (1) vmss.name uses attribute access on a dict and will raise AttributeError — should be vmss['name']. (2) vmss.sku.capacity in the error message on line 697 also uses attribute access on a dict — should be vmss['sku']['capacity'].
| extension_type = ext.type_properties_type.lower() | ||
| if ext.get('type1'): | ||
| extension_type = ext['type1'].lower() | ||
| if ext.get('type'): |
There was a problem hiding this comment.
The second condition changed from elif to if. This means that if both type1 and type are present, type will always overwrite the value set by type1. While this may be the intended behavior, it's a subtle logic change from the original code where type_properties_type was only checked if type1 was absent. Consider whether you intended to keep the elif pattern to preserve the original precedence logic, or if you intentionally want type to take priority.
Suggested change
| if ext.get('type'): | |
| elif ext.get('type'): |
| extension.type = AAZUndefined | ||
|
|
||
| return self.deserialize_output(self.ctx.vars.instance, client_flatten=True) | ||
|
|
There was a problem hiding this comment.
Missing blank line before def convert_show_result_to_snake_case. PEP 8 requires two blank lines between top-level definitions. This was likely caused by the insertion of the VMSSList class above.
Suggested change
Comment on lines
+694
to
+706
| if vmss.get('sku', {}).get('capacity') and vmss.get('sku', {}).get('capacity') < instance_target: | ||
| if auto_add_node is not True: | ||
| raise CLIError('Please use --auto_add_node to automatically increase the nodes,{} requires {} nodes, but currenty there are {}'. | ||
| format(reliability_level, instance_target, vmss.sku.capacity)) | ||
| vmss.sku.capacity = instance_target | ||
| vmss_poll = compute_client.virtual_machine_scale_sets.begin_create_or_update( | ||
| resource_group_name, vmss.name, vmss) | ||
| vmss['sku']['capacity'] = instance_target | ||
| from ..vm.operations.vmss import VMSSCreate | ||
| vmss['resource_group'] = resource_group_name | ||
| vmss['vm_scale_set_name'] = vmss.name | ||
| vmss_poll = VMSSCreate(cli_ctx=cmd.cli_ctx)(command_args=vmss) | ||
| LongRunningOperation(cli_ctx)(vmss_poll) | ||
|
|
||
| node_type.vm_instance_count = vmss.sku.capacity | ||
| if vmss.get('sku', {}).get('capacity'): | ||
| node_type.vm_instance_count = vmss['sku']['capacity'] |
There was a problem hiding this comment.
The truthiness check vmss.get('sku', {}).get('capacity') will evaluate to False when capacity is 0, changing the behavior compared to the original code which simply did vmss.sku.capacity < instance_target. While a VMSS with 0 capacity may be uncommon, consider using an explicit is not None check instead: vmss.get('sku', {}).get('capacity') is not None and vmss['sku']['capacity'] < instance_target. The same concern applies to line 705 where node_type.vm_instance_count assignment would be skipped if capacity is 0.
|
|
||
| vmss['sku']['capacity'] = vmss['sku']['capacity'] + number_of_nodes_to_add | ||
| vmss['resource_group'] = resource_group_name | ||
| vmss['vm_scale_set_name'] = vmss.name |
There was a problem hiding this comment.
Two issues: (1) vmss.name uses attribute access on a dict, which will raise AttributeError. (2) Even after fixing to vmss['name'], this will raise KeyError because convert_show_result_to_snake_case (called in _get_cluster_vmss_by_node_type) does not copy the name field to the new result dict. The name field needs to be preserved in convert_show_result_to_snake_case, or accessed from the original result before conversion. The same issue applies at lines 515, 567, 701, and 1248.
Suggested change
| vmss['vm_scale_set_name'] = vmss.name | |
| vmss['vm_scale_set_name'] = vmss.get('name') |
| vmss_poll = compute_client.virtual_machine_scale_sets.begin_create_or_update( | ||
| resource_group_name, vmss.name, vmss) | ||
| vmss['resource_group'] = resource_group_name | ||
| vmss['vm_scale_set_name'] = vmss.name |
There was a problem hiding this comment.
vmss.name uses attribute access on a dict, which will raise AttributeError. Should be vmss['name'].
| poller = compute_client.virtual_machine_scale_sets.begin_create_or_update( | ||
| resource_group_name, vmss.name, vmss) | ||
| vmss['resource_group'] = resource_group_name | ||
| vmss['vm_scale_set_name'] = vmss.name |
There was a problem hiding this comment.
vmss.name uses attribute access on a dict, which will raise AttributeError. Should be vmss['name'].
Suggested change
| vmss['vm_scale_set_name'] = vmss.name | |
| vmss['vm_scale_set_name'] = vmss['name'] |
Comment on lines
1234
to
1245
| @@ -1184,19 +1244,20 @@ def _add_cert_to_vmss(cli_ctx, vmss, resource_group_name, vault_id, secret_url): | |||
| secrets[0].vault_certificates.append( | |||
| VaultCertificate(secret_url, 'my')) | |||
There was a problem hiding this comment.
The else branch still uses SDK-style attribute access (secrets[0].vault_certificates, c.certificate_url, VaultCertificate(...)) on objects that are now plain dicts returned by the AAZ command. These will raise AttributeError. They need to be converted to dict-style access (e.g., secrets[0]['vault_certificates'], c['certificate_url']) and dict literals instead of VaultCertificate(...) constructor calls.
| json_data['storageAccountKey'] = list_results.keys[0].value | ||
| json_data['storageAccountEndPoint'] = "https://core.windows.net/" | ||
| diagnostics_ext.protected_settings = json_data | ||
| diagnostics_ext['protectedSettings'] = json_data |
There was a problem hiding this comment.
After convert_show_result_to_snake_case is applied (via _get_cluster_vmss_by_node_type), the extension dict uses snake_case keys, so protectedSettings has been renamed to protected_settings. Using camelCase protectedSettings here adds a new key that the AAZ VMSSCreate command may not recognize. This should be diagnostics_ext['protected_settings'] = json_data to be consistent with lines 1025-1027 which correctly use protected_settings.
Suggested change
| diagnostics_ext['protectedSettings'] = json_data | |
| diagnostics_ext['protected_settings'] = json_data |
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
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 sf cluster reliability updateaz sf cluster durability updateaz sf cluster node-type addaz sf cluster node addaz sf cluster node removesf application certificate addDescription
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.