-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds progress bar for image push #421
Adds progress bar for image push #421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some comments on the PR it looks good. Let us try to see if we get these tests to pass
4129005
to
366e1ae
Compare
2e32e94
to
e7166ad
Compare
433cfcc
to
41f257a
Compare
7bf316a
to
bbcc0d1
Compare
// Start the display of the Progress Bar | ||
func (l *ProgressBarLogger) Start(ctx context.Context, progressChan <-chan regv1.Update) { | ||
ctx, cancelFunc := context.WithCancel(ctx) | ||
l.cancelFunc = cancelFunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im assuming we are not using ProgressBarLogger concurrently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we are not using
pkg/imgpkg/registry/with_progress.go
Outdated
@@ -4,6 +4,8 @@ | |||
package registry | |||
|
|||
import ( | |||
context "context" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: redundant aliasing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove the alias
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the latest commit.
pkg/imgpkg/plainimage/contents.go
Outdated
excludedPaths []string | ||
paths []string | ||
excludedPaths []string | ||
displayProgress bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we wouldn't need the displayProgress flag as it is not used anymore, however we would need the paths and excluded paths. I will remove the displayProgress flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the displayProgress as part of the latest commit
if l.cancelFunc != nil { | ||
l.cancelFunc() | ||
} | ||
if l.ui != nil && l.finalMessage != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to check on l.ui != nil
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of a NOOP progress logger
as part of the bundle push , we are not passing any UI as there will not be any progress bar displaying. However we could have the same accepting ui as well and we would need the nil
check to decide if possible to display final message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR resolves Issue #420 to have progress bar displayed for Image push.