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>
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the VM command modules by introducing additional debugging/test variables in custom.py and adding new arguments in _params.py for disk bursting.
- Adds temporary test variables and a test class in custom.py.
- Introduces two new arguments for disk bursting in _params.py and updates help text for certain 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 | Adds test variables and a test class; potential naming issues. |
| src/azure-cli/azure/cli/command_modules/vm/_params.py | Introduces new arguments for disk bursting with similar help text. |
Comments suppressed due to low confidence (5)
src/azure-cli/azure/cli/command_modules/vm/custom.py:856
- [nitpick] Rename variable 'testPort2' to 'test_port2' to adhere to snake_case naming conventions.
testPort2 = 8811
src/azure-cli/azure/cli/command_modules/vm/custom.py:5987
- [nitpick] Rename class 'test_PR_review' to 'TestPrReview' to follow CamelCase naming for classes.
class test_PR_review:
src/azure-cli/azure/cli/command_modules/vm/custom.py:5988
- [nitpick] Rename method 'testPPReviw' to 'test_pr_review' to comply with snake_case naming and correct the spelling of 'review'.
def testPPReviw(self):
src/azure-cli/azure/cli/command_modules/vm/_params.py:157
- [nitpick] Consider using a more descriptive name than 'enable_bursting1' to clarify its purpose.
c.argument('enable_bursting1', 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:158
- [nitpick] Consider using a more descriptive name than 'enable_bursting2' to clarify its purpose.
c.argument('enable_bursting2', arg_type=get_three_state_flag(), help='Enabled 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.')
There was a problem hiding this comment.
Pull Request Overview
This PR adds temporary/test code to the VM custom module and introduces two new CLI parameters with help messages to the VM parameters file.
- Injects debug/test variables and a test class into
custom.py, which appears unintended. - Adds
enable_bursting1andenable_bursting2parameters with help text in_params.py. - Tweaks existing help messages for consistency.
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/custom.py | Added unused debug/test variables (i, j, test_port, etc.) and a stray test class at end of file. |
| src/azure-cli/azure/cli/command_modules/vm/_params.py | Introduced enable_bursting1/enable_bursting2 arguments; their help messages use incorrect verb forms. |
| key_incarnation_id=None): | ||
| i = 8808 | ||
| j = 8809 | ||
| test_port = 8810 |
There was a problem hiding this comment.
This test_port variable is defined but never used; please remove it.
| test_port = 8810 |
| c.argument('network_access_policy', min_api='2020-05-01', help='Policy for accessing the disk via network.', arg_type=get_enum_type(self.get_models('NetworkAccessPolicy', operation_group=operation_group))) | ||
| c.argument('disk_access', min_api='2020-05-01', help='Name or ID of the disk access resource for using private endpoints on disks.') | ||
| c.argument('enable_bursting', arg_type=get_three_state_flag(), help='Enable 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.') | ||
| c.argument('enable_bursting1', 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.') |
There was a problem hiding this comment.
Help message starts with third-person verb 'Enables'; use active-voice imperative (e.g., 'Enable on-demand bursting...').
| c.argument('enable_bursting1', 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.') | |
| c.argument('enable_bursting1', arg_type=get_three_state_flag(), help='Enable 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.') |
| c.argument('disk_access', min_api='2020-05-01', help='Name or ID of the disk access resource for using private endpoints on disks.') | ||
| c.argument('enable_bursting', arg_type=get_three_state_flag(), help='Enable 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.') | ||
| c.argument('enable_bursting1', 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.') | ||
| c.argument('enable_bursting2', arg_type=get_three_state_flag(), help='Enabled 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.') |
There was a problem hiding this comment.
Help message uses passive voice 'Enabled'; switch to active-voice imperative (e.g., 'Enable on-demand bursting...').
| c.argument('enable_bursting2', arg_type=get_three_state_flag(), help='Enabled 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.') | |
| c.argument('enable_bursting2', arg_type=get_three_state_flag(), help='Enable 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.') |
| 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.
Remove this debug variable (i) from production code or rename it to something meaningful if intended for real logic.
| 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.
Remove this debug variable (j) from production code or rename it to something meaningful if intended for real logic.
| i = 8808 | ||
| j = 8809 | ||
| test_port = 8810 | ||
| testPort2 = 8811 |
There was a problem hiding this comment.
Variable name testPort2 uses camelCase and doesn’t follow snake_case convention; also it’s unused and should be removed.
| j = 8809 | ||
| test_port = 8810 | ||
| testPort2 = 8811 | ||
| testPort3 = (12*23+14) |
There was a problem hiding this comment.
Variable testPort3 both uses camelCase and contains a magic calculation; remove or clarify purpose and use snake_case.
| test_port = 8810 | ||
| testPort2 = 8811 | ||
| testPort3 = (12*23+14) | ||
| test_str= 'test' |
There was a problem hiding this comment.
Extraneous whitespace around the assignment operator; should be formatted as test_str = 'test'.
| # endRegion | ||
|
|
||
|
|
||
| class test_PR_review: |
There was a problem hiding this comment.
Class name test_PR_review does not follow CamelCase convention; rename to something like TestPrReview or remove stray test class.
|
|
||
|
|
||
| class test_PR_review: | ||
| def testPPReviw(self): |
There was a problem hiding this comment.
Method name testPPReviw is misspelled and not snake_case; rename to test_pr_review or remove if unintended.
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.