Update spinner API to stop all running spinners#170
Update spinner API to stop all running spinners#170chandrareddyp merged 1 commit intovmware-tanzu:mainfrom
Conversation
chandrareddyp
force-pushed
the
update/spinner-signal-api
branch
from
3d98733 to
1863117
Compare
| for _, s := range spinners { | ||
| if s != nil { | ||
| if s.GetErrorText() != "" { | ||
| s.SetFinalText(s.GetErrorText(), log.LogTypeERROR) | ||
| } | ||
| if ows.spinner != nil && ows.spinner.Active() { | ||
| ows.spinner.Stop() | ||
| if ows.spinnerFinalText != "" { | ||
| fmt.Fprintln(ows.out) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We should not be updating this function. This function is to stop specific spinner and should be kept as it is.
We should implement a separate public API like StopAllSpinners (or something similar) which invokes StopSpinner.
There was a problem hiding this comment.
The API consumer may confuse that why should he call StopAllSpinners as he started a single spinner! If we give a option StopSpinner API then they may use just StopSpinner API instead of StopAllSpinners.
There was a problem hiding this comment.
@chandrareddyp users will not have access to the created outputwriterspinner object while handling the interrupt and hence this cannot be even invoked.
TPR does need to provide an API to stop all spinner objects because TPR can keep track of all open spinner and close them when needed. The way it is implemented today, It does not make much sense to close all spinners by invoking some random spinner object's StopSpinner API.
|
Can you update the PR title and PR description? |
chandrareddyp
changed the title
Updated |
|
Please add a release note for this PR as well. |
marckhouzam
left a comment
marckhouzam
left a comment
There was a problem hiding this comment.
The code LGTM but could you put in the PR description what scenario we are trying to address? I believe it is for interrupts, right? An example of how this would be used would help. Currently, it is hard to see the added benefit of this new function.
chandrareddyp
force-pushed
the
update/spinner-signal-api
branch
2 times, most recently
from
6f68d37 to
1b66137
Compare
chandrareddyp
force-pushed
the
update/spinner-signal-api
branch
from
1b66137 to
3f3b1dc
Compare
4 participants
What this PR does / why we need it
This PR updates the spinner API to store all spinners and close any that are still active when the user wants to stop the spinner after the command has completed or been interrupted. This will help ensure all spinners are properly closed.
Use case:
If the user is running
tanzu plugin sync, it installs all the required plugins for each context. Assume that it has installed a few plugins, and now the user wants to terminate the command with Ctrl+C. In this case, the API should stop the current spinner and also check for any other active spinners and terminate them as well. This new API helps handle these types of use cases.Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer