[AKS] az aks command invoke: Add progress spinner
#30274
Conversation
CustardTart32
requested review from
zhoxing-ms,
jsntcy,
yanzhudd,
jiasli,
evelyn-ys,
kairu-ms,
calvinhzy,
necusjz and
bebound
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @CustardTart32, |
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
yonzhan and
FumingZhang
microsoft-github-policy-service
bot
added
Auto-Assign
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Queued live test to validate the change, test passed!
- test_aks_run_command
| @@ -2058,7 +2102,8 @@ def aks_runcommand(cmd, client, resource_group_name, name, command_string="", co | |||
| command_id = command_id_regex.findall(command_result_polling_url)[0] | |||
| _aks_command_result_in_progess_helper(client, resource_group_name, name, command_id) | |||
| return | |||
| return _print_command_result(cmd.cli_ctx, command_result_poller.result(300)) | |||
|
|
|||
| return LongRunningOperation(cmd.cli_ctx, progress_bar=PollerProgressBar(cmd.cli_ctx, command_result_poller))(command_result_poller) | |||
There was a problem hiding this comment.
Seems this would change the command output, and this might be regarded as a breaking change.
There was a problem hiding this comment.
It would be great if you could add an example in the description to show the difference before and after the change.
There was a problem hiding this comment.
Yep, just checked and this would change the output format. I'll see if I can align the two.
There was a problem hiding this comment.
Fixed, I've added a new commit to align the output formats.
After:
CustardTart32
force-pushed
the
czhong/feat/run-command-status-poller
branch
from
12712f0
to
67b9c42
Compare
CustardTart32
force-pushed
the
czhong/feat/run-command-status-poller
branch
2 times, most recently
from
75d5f38
to
7bbfe83
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
please fix the title with impacted command/parameter, e.g., |
CustardTart32
force-pushed
the
czhong/feat/run-command-status-poller
branch
from
d4f839d
to
d2f3cd6
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
please add tests for the change to verify if it meets your expectations |
| progress_bar.end() | ||
| return _print_command_result(cmd.cli_ctx, command_result_poller.result()) |
There was a problem hiding this comment.
so if the operation is not completed within 300 seconds, the progress bar will end, but the polling here will continue (until it times out on the server side?)
There was a problem hiding this comment.
From what I understand, if the operation is not completed in 300s, the failure message will be logged and the python process (including the poller) will be terminated.
There was a problem hiding this comment.
I did a simple test based on your branch, the following command is executed and does not terminate after 5 minutes. After 5 minutes, only the progress bar is no longer displayed, and the command finally returns in 10 minutes.
az aks command invoke -g xxx -n xxx -c "sleep 600 && echo hello"
|
|
||
|
|
||
| class RunCommandLocationPolling(LocationPolling): | ||
| """Extends LocationPolling but uses the body content instead of the status code for the status""" |
There was a problem hiding this comment.
May I ask why you want to inherit your own Poller and use this special status logic?
There was a problem hiding this comment.
Basically, I want the message in the progress bar to be dynamically generated based on the poller HTTP response. LocationPolling basically checks the status code and returns a hardcoded value of InProgress if HTTP status 201/202 hence why we have to extend the class.
BodyContentPolling only supports PUT/PATCH methods which is not applicable here and neither does OperationResourcePolling.
There was a problem hiding this comment.
Running is the status field from the API response.
CustardTart32
force-pushed
the
czhong/feat/run-command-status-poller
branch
from
e40f4dd
to
dbe98d0
Compare
|
Thanks @CustardTart32 , this is first step on AKS to show more details on long running operations, to improve customer experience. here I capture a gif using asciinema, you can see the Later we are going to apply this to |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…eterminateProgressBar
CustardTart32
force-pushed
the
czhong/feat/run-command-status-poller
branch
from
dbe98d0
to
2c2b08e
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| now = datetime.datetime.now() | ||
| progress_controller.begin() | ||
| while not command_result_poller.done(): | ||
| if datetime.datetime.now() - now >= datetime.timedelta(seconds=300): | ||
| break | ||
|
|
||
| progress_controller.add(message=command_result_poller.status()) | ||
| progress_controller.update() | ||
| time.sleep(0.5) | ||
|
|
||
| progress_controller.end() | ||
| return _print_command_result(cmd.cli_ctx, command_result_poller.result()) |
There was a problem hiding this comment.
#30274 (comment)
My suggestion is to either share this timeout with both the progress bar and the poller, or not set a timeout.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Related command
az aks command invoke
Description
This PR adds a spinner and progress message to the
az aks command invokeCLI command, which can take several minutes to complete.Testing Guide
History Notes
[AKS]
az aks command invoke: Add progress spinnerThis 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.