Conversation
zhoxing-ms
requested review from
jsntcy,
wangzelin007 and
yanzhudd
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @zhoxing-ms, |
️✔️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>
|
zhoxing-ms
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR makes modifications for the Azure Virtual Machines command modules.
- In custom.py, temporary test variable assignments have been introduced.
- In _params.py, help messages for several VM arguments have been updated for clarity.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/vm/custom.py | Added test variable assignments with hard-coded numbers that may be temporary or debugging artifacts. |
| src/azure-cli/azure/cli/command_modules/vm/_params.py | Updated help text wording for VM argument definitions. |
| exclude_zones=None, align_regional_disks_to_vm_zone=None, wire_server_mode=None, imds_mode=None, | ||
| wire_server_access_control_profile_reference_id=None, imds_access_control_profile_reference_id=None, | ||
| key_incarnation_id=None): | ||
| i = 8808 |
There was a problem hiding this comment.
The variable 'i' with a hard-coded number appears to be a placeholder or testing artifact. Consider renaming or removing it if it is not intended for production use.
| i = 8808 |
| wire_server_access_control_profile_reference_id=None, imds_access_control_profile_reference_id=None, | ||
| key_incarnation_id=None): | ||
| i = 8808 | ||
| j = 8809 |
There was a problem hiding this comment.
The variable 'j' with a literal value is unclear in purpose. Please document its usage or remove it if it's leftover testing code.
| j = 8809 |
| key_incarnation_id=None): | ||
| i = 8808 | ||
| j = 8809 | ||
| test_port = 8810 |
There was a problem hiding this comment.
The use of a hard-coded number for 'test_port' can make future maintenance more challenging. Consider parameterizing this value or adding inline documentation to clarify its purpose.
| test_port = 8810 | |
| test_port = DEFAULT_TEST_PORT |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces temporary test variables in the VM creation logic and refines several help strings for VM CLI parameters.
- Adds unused debugging variables (
i,j,test_port) increate_vm - Updates help text wording for bursting, availability-set, and disk-caching parameters
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/vm/custom.py | Inserts debugging placeholders (i, j, test_port) |
| src/azure-cli/azure/cli/command_modules/vm/_params.py | Tweaks help descriptions for enable_bursting, platform_update_domain_count, and disk_caching |
Comments suppressed due to low confidence (4)
src/azure-cli/azure/cli/command_modules/vm/custom.py:854
- Remove these unused test variables (
i,j, andtest_port) as they appear to be debugging artifacts.
i = 8808
src/azure-cli/azure/cli/command_modules/vm/_params.py:156
- [nitpick] Use imperative voice for help text to maintain consistency; revert 'Enables' back to 'Enable'.
c.argument('enable_bursting', arg_type=get_three_state_flag(), help='Enables on-demand bursting beyond the provisioned performance target of the disk. On-demand bursting is disabled by default, and it does not apply to Ultra disks.')
src/azure-cli/azure/cli/command_modules/vm/_params.py:392
- The phrase 'Updated Domain count' is a typo; it should read 'Update Domain count'.
c.argument('platform_update_domain_count', type=int, help='Updated Domain count. If unspecified, the server will pick the most optimal number like 5.')
src/azure-cli/azure/cli/command_modules/vm/_params.py:419
- [nitpick] The revised help text is less clear and inconsistent with other parameters. Consider restoring the original phrasing ('Use singular value to apply across...') to improve clarity.
c.argument('disk_caching', nargs='*', help="The singular value to apply across, or specify individual disks, e.g. 'os=ReadWrite 0=None 1=ReadOnly' should enable update os disk and 2 data disks")
Related command
Description
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.