Conversation
xuexu6666
requested review from
jsntcy,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @xuexu6666, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| aks nodepool delete-machines | cmd aks nodepool delete-machines added |
Collaborator
|
AKS |
microsoft-github-policy-service
bot
requested review from
FumingZhang and
yonzhan
microsoft-github-policy-service
bot
added
Auto-Assign
xuexu6666
changed the title
az aks nodepool delete-machines
FumingZhang
reviewed
Sep 18, 2024
| def aks_machine_list_table_format(results): | ||
| return [aks_machine_show_table_format(r) for r in results] | ||
|
|
||
| def aks_machine_show_table_format(result): |
Member
There was a problem hiding this comment.
Can you add some unit tests for this?
Contributor
Author
There was a problem hiding this comment.
I finally decide to use kubectl to get machine name instead of use machine api. Machine api is not GA yet.
| @AKSCustomResourceGroupPreparer( | ||
| random_name_length=17, name_prefix="clitest", location="westus2" | ||
| ) | ||
| def test_aks_nodepool_delete_machines(self, resource_group, resource_group_location): |
Member
There was a problem hiding this comment.
Queued live test for this, you'll need to commit the recording file (would be generated by running the test case in live mode, find it from pipeline artifact) to pass CI.
Contributor
Author
There was a problem hiding this comment.
https://dev.azure.com/msazure/CloudNativeCompute/_build/results?buildId=103633419&view=results
This is the passed test I ran. Wondering which recording file do I need? btw, I uploaded the test_aks_nodepool_delete_machines.yaml already.
Member
There was a problem hiding this comment.
Take another look, it seems that you performed some data plane operations in the test case, which cannot be properly replayed. Please mark the case as @live_only() (and left a comment, remove the corresponding recording file) to bypass the check.
FumingZhang
reviewed
Sep 20, 2024
Member
FumingZhang
left a comment
FumingZhang
left a comment
There was a problem hiding this comment.
Please fix failed CI checks
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/_params.py:924:1: E302 expected 2 blank lines, found 1
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/custom.py:2651:1: E302 expected 2 blank lines, found 1
| g.custom_command('start', 'aks_agentpool_start', supports_no_wait=True) | ||
| g.wait_command('wait') | ||
| g.custom_command('operation-abort', 'aks_agentpool_operation_abort', supports_no_wait=True) | ||
| g.custom_command("delete-machines", "aks_agentpool_delete_machines", supports_no_wait=True) |
Member
There was a problem hiding this comment.
- FAIL - HIGH severity: missing_command_test_coverage
Repo: ./, Src Branch: HEAD, Target Branch: origin/dev - Failed.
Missing command test coverage:aks nodepool "delete-machines"
Please add some scenario tests for the new command
Or add the command with missing_command_test_coverage rule in linter_exclusions.yml
Rules in CI might be a bit rigid, I guess changing "delete-machines" to 'delete-machines' would fix the issue.
| @AKSCustomResourceGroupPreparer( | ||
| random_name_length=17, name_prefix="clitest", location="westus2" | ||
| ) | ||
| def test_aks_nodepool_delete_machines(self, resource_group, resource_group_location): |
Member
There was a problem hiding this comment.
Take another look, it seems that you performed some data plane operations in the test case, which cannot be properly replayed. Please mark the case as @live_only() (and left a comment, remove the corresponding recording file) to bypass the check.
FumingZhang
approved these changes
Sep 20, 2024
Member
|
It is recommended to modify the PR title to |
yanzhudd
changed the title
az aks nodepool delete-machinesaz aks nodepool delete-machines: Add support to delete specific machines in an agent pool
yanzhudd
approved these changes
Sep 24, 2024
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
5 participants
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 aks nodepool delete-machines
delete machines API was added to 2024-07-01
Description
We want to add the new feature delete-machines in GA cli.
Testing Guide
Use the command
az aks nodepool delete-machinesto send the request to aks. Use the commandaz aks machine listto verify the results.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.