new(cmd,pkg/driver): properly use a spinner for long operations. #366
Conversation
| @@ -317,7 +314,7 @@ func (o *artifactInstallOptions) RunArtifactInstall(ctx context.Context, args [] | |||
| logger.Info("Extracting and installing artifact", logger.Args("type", result.Type, "file", result.Filename)) | |||
|
|
|||
| if !o.Printer.DisableStyling { | |||
| sp, _ = o.Printer.Spinner.Start("Extracting and installing") | |||
| o.Printer.Spinner, _ = o.Printer.Spinner.Start("Extracting and installing") | |||
There was a problem hiding this comment.
We need to save the spinner to our o.Printer one to let main properly stop the right spinner when a context error happens.
cmd/driver/cleanup/cleanup.go
Outdated
| // Print much more readable output as-is | ||
| o.Printer.DefaultText.Print(buf.String()) | ||
| } | ||
| buf.Reset() |
There was a problem hiding this comment.
This is not needed here since it is not reused.
I think it is safer to let it there though.
| @@ -40,6 +40,9 @@ func main() { | |||
| if opt.Printer != nil && opt.Printer.ProgressBar != nil && opt.Printer.ProgressBar.IsActive { | |||
| _, _ = opt.Printer.ProgressBar.Stop() | |||
| } | |||
| if opt.Printer != nil && opt.Printer.Spinner != nil && opt.Printer.Spinner.IsActive { | |||
There was a problem hiding this comment.
If there's a spinner in use, stop it upon leaving.
|
/milestone v0.7.0 |
| if o.Printer.Spinner != nil { | ||
| _ = o.Printer.Spinner.Stop() | ||
| } | ||
| if o.Printer.Logger.Formatter == pterm.LogFormatterJSON { |
There was a problem hiding this comment.
This is ugly: we should probably find a better way to deal with unformatted logs while in JSON mode.
cmd/driver/install/install.go
Outdated
| if !o.Printer.DisableStyling { | ||
| o.Printer.Spinner, _ = o.Printer.Spinner.Start("Cleaning up existing drivers") | ||
| } | ||
| err = driver.Type.Cleanup(o.Printer.WithWriter(&buf), driver.Name) |
There was a problem hiding this comment.
Idea is pretty simple: we start the spinner and to avoid called commands to print anythng to screen, we pass to them a new printer variable that prints onto a buffer.
After we stop the spinner, we are free to print all messages from the buffer.
Signed-off-by: Federico Di Pierro <[email protected]>
FedeDP
force-pushed
the
new/driver_use_spinner
branch
from
e3fcfd5 to
b5505c2
Compare
Signed-off-by: Federico Di Pierro <[email protected]>
FedeDP
force-pushed
the
new/driver_use_spinner
branch
from
b5505c2 to
d1d9131
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, leogr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area library
/area cli
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: