feat: add/improve spinner for build and deploy #322
Conversation
lance
force-pushed
the
296-spinner
branch
2 times, most recently
from
915b89b to
2d6802a
Compare
lance
changed the title
This commit modifies the progress meter so that, by default there is no step counter. It also modifies the responsibility for calling `Done()`, making it the job of the command rather than the client. This is because the client does not know how many commands will be executed and therefore cannot know when the progress bar is done. Fixes: knative#296 Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
This commit adds String() to the progress bar, and moves logging responsibility out of the deployer itself and fully into the client. Deployer#Deploy() now returns a DeploymentResult. Signed-off-by: Lance Ball <lball@redhat.com>
|
@lkingland thanks for your suggestions. I've incorporated them. PTAL |
Signed-off-by: Lance Ball <lball@redhat.com>
|
Why does this contains |
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
Hmm - it shouldn't have - and no longer does. |
| ) | ||
|
|
||
| type DeploymentResult struct { | ||
| Status Status |
There was a problem hiding this comment.
I also like this because it helps us maintain backwards compatibility
| @@ -16,7 +16,7 @@ import ( | |||
| servingv1 "knative.dev/serving/pkg/apis/serving/v1" | |||
| v1 "knative.dev/serving/pkg/apis/serving/v1" | |||
|
|
|||
| bosonFunc "github.com/boson-project/func" | |||
| fn "github.com/boson-project/func" | |||
There was a problem hiding this comment.
certainly liking this import name better
| // 5 Print linebreak such that subsequent messaes print correctly. | ||
| fmt.Fprintf(b.out, "\r%v%v%v%v/%v %v\n", up, clear, prefix, b.index, b.total, b.text) | ||
| // 5 Print linebreak such that subsequent messages print correctly. | ||
| fmt.Fprintf(b.out, "\r%v%v%v%v\n", up, clear, prefix, b) |
There was a problem hiding this comment.
Ah hah, handy use of b's .String already there.
progress/progress.go
Outdated
| // Start the spinner if not already started | ||
| if b.ticker == nil { | ||
| fmt.Println() | ||
| b.ticker = time.NewTicker(100 * time.Millisecond) | ||
| go b.spin(b.ticker.C) | ||
| } | ||
|
|
There was a problem hiding this comment.
I see this is moved such that now the spiner is always running, even in verbose mode (but as a noop due to the verbosity check being moved to the spinner's write). I'm interested to know what bug this fixed, because fiddling with the line status is finnicky indeed, and of course it would be ideal to not start the goroutine if it's never going to be used.
There was a problem hiding this comment.
Yes - it is absolutely fiddly messing with the line buffer! Oddly, with some of the other changes I made, the spinner ended up on the next line, e.g.
Building function image
/ <- that's the spinnerGood point about not starting the goroutine if we don't have to. I'll fiddle with this a bit.
There was a problem hiding this comment.
Oh and also - if we call overwrite() first, we end up overwriting the user command. When I type
$ # showing previous line for illustration
$ kn func build<enter>We see this (you can see this with the current release binary).
$ # showing previous line for illustration
Building function image
/ <- that's the spinnerNote that the kn func build command and the $ prompt are overwritten.
Signed-off-by: Lance Ball <lball@redhat.com>
This commit modifies the progress meter so that, by default there is no
step counter. It also modifies the responsibility for calling
Done(),making it the job of the command rather than the client. This is because
the client does not know how many commands will be executed and therefore
cannot know when the progress bar is done.
Fixes: #296
Signed-off-by: Lance Ball lball@redhat.com