Skip to content

Conversation

@Juliehzl
Copy link
Contributor

@Juliehzl Juliehzl commented Mar 10, 2021

Description

This PR is one part for progress bar project.
As we already in beta version for several months, I plan to move indeterminate progress bar with spinner to all long running operation in azure cli as shown below:
image

But I have to mention for commands like network vnet create, which is long running operation, but not hornor CLI LongRunningOperation class, will not have progress bar.

Testing Guide

az storage account create
az vm create

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 feature.


This checklist is used to make sure that common guidelines for a pull request are followed.

@Juliehzl Juliehzl requested a review from zhoxing-ms as a code owner March 10, 2021 08:58
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 10, 2021

Core

@yonzhan yonzhan added this to the S184 milestone Mar 10, 2021
class IndeterminateProgressBar:
""" Define progress bar update view """
def __init__(self, cli_ctx, message="Running"):
self.cli_ctx = cli_ctx
Copy link
Member

@jiasli jiasli Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider dropping cli_ctx reference if it is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in L181 to get progress controller

@evelyn-ys
Copy link
Member

evelyn-ys commented Mar 18, 2021

I compared the implement before and found that the original way support spinner already. There's just a bug winthin LongRunningOperation. L123 shows that cli_ctx.get_progress_controller() will init progress and create a new spinner each time when called.

def get_progress_controller(self, det=False):
import azure.cli.core.commands.progress as progress
if not self.progress_controller:
self.progress_controller = progress.ProgressHook()
self.progress_controller.init_progress(progress.get_progress_view(det))
return self.progress_controller

LRO calls self.cli_ctx.get_progress_controller().begin(), self.cli_ctx.get_progress_controller().add(message='Running') or self.cli_ctx.get_progress_controller().end() to update progress. But each time updating the progress, the progress will be initialized. So the behavior will look like no animation.

To support spinning for LRO, we only need to log progress_controller = self.cli_ctx.get_progress_controller() and then call progress_controller.begin(), progress_controller.add(message='Running') and progress_controller.end() within LongRunningOperation.

@yonzhan yonzhan modified the milestones: S184, S185 Mar 20, 2021
@Juliehzl
Copy link
Contributor Author

I compared the implement before and found that the original way support spinner already. There's just a bug winthin LongRunningOperation. L123 shows that cli_ctx.get_progress_controller() will init progress and create a new spinner each time when called.

def get_progress_controller(self, det=False):
import azure.cli.core.commands.progress as progress
if not self.progress_controller:
self.progress_controller = progress.ProgressHook()
self.progress_controller.init_progress(progress.get_progress_view(det))
return self.progress_controller

LRO calls self.cli_ctx.get_progress_controller().begin(), self.cli_ctx.get_progress_controller().add(message='Running') or self.cli_ctx.get_progress_controller().end() to update progress. But each time updating the progress, the progress will be initialized. So the behavior will look like no animation.

To support spinning for LRO, we only need to log progress_controller = self.cli_ctx.get_progress_controller() and then call progress_controller.begin(), progress_controller.add(message='Running') and progress_controller.end() within LongRunningOperation.

Yes, there is an issue in previous design. Here I don't use add() because I want to expand progress bar capability with update_progress(), which could have more progress bar UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants