feat: track progress on image pull/push #3904
Conversation
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
src/internal/packager/images/pull.go
Outdated
There was a problem hiding this comment.
It's easier to review this with whitespace hidden
| const defaultProgressInterval = 5 * time.Second | ||
|
|
||
| // StartReporting starts the reporting goroutine | ||
| func (tt *TrackedTarget) StartReporting() { |
There was a problem hiding this comment.
Given that the push is governed by the caller-supplied context - I wonder if this should accept context and if we could remove more of the bespoke stopReports in favor of a ctx.Done()
There was a problem hiding this comment.
Yeah it should definitely accept a context. Though to get rid of the StopReporting call we'd have to make a separate context for StartReporting then cancel it. I like the feel of the defer StopReporting slightly more, but I don't feel too strongly either way
|
|
||
| const defaultProgressInterval = 5 * time.Second | ||
|
|
||
| // StartReporting starts the reporting goroutine |
There was a problem hiding this comment.
What happens if StartReporting is called twice? is there any potential state that needs to be tracked?
There was a problem hiding this comment.
It would duplicate the log calls. Not sure if I should add a boolean like reportingStarted on the Tracker object and return if so.
On one hand we wouldn't want duplicate logging calls to happen for the same object. On the other hand, there is no situation where startReporting should be called twice without StopReporting called inbetween, so having the check for duplicate calls makes it easier for mistakes like that to go unnoticed. LMK what you think.
| } | ||
|
|
||
| // NewTrackedTarget creates a new TrackedTarget | ||
| func NewTrackedTarget(target oras.Target, totalBytes int64, reporter Report) *TrackedTarget { |
There was a problem hiding this comment.
Is there any value in making interval configurable?
There was a problem hiding this comment.
Maybe in the future, given that this is internal and we want the same interval everywhere. I didn't expose the interval
|
|
||
| // Push wraps the target push method with an appropriate tracked reader. | ||
| func (tt *TrackedTarget) Push(ctx context.Context, desc ocispec.Descriptor, content io.Reader) error { | ||
| tReader := &trackedReader{ |
There was a problem hiding this comment.
Maybe a nit - I think we could potentially do without the temporary trackedReader
if wt, ok := content.(io.WriterTo); ok {
tracked := &trackedWriterToReader{writerTo: wt, bytesRead: &tt.bytesRead}
return tt.Target.Push(ctx, desc, tracked)
}
return tt.Target.Push(ctx, desc, &trackedReader{reader: content, bytesRead: &tt.bytesRead})
There was a problem hiding this comment.
I like embedding the trackedReader into the trackedWritertoReader struct this way we don't have to maintain another Read method on the trackedWritertoReader, it'll just use the trackedReader object.
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
|
@brandtkeller Separated out the Tracker object and created a unit test for it. I wanted to avoid creating a registry or mocking an |
| ) | ||
|
|
||
| // newTestTracker creates a Tracker instance with a customizable report interval for testing. | ||
| func newTestTracker(bytesRead, totalBytes int64, reporter Report, interval time.Duration) *Tracker { |
Description
This introduces occasional log statements to give progress during OCI pulls and pushes (images and Zarf packages).
Implementation was loosely inspired by the progressbar implementation in the ORAS CLI. Goal was to create simple a simple solution that avoids leaving users without feedback from the system for more than five seconds. UX was designed to be similar to how opentofu gives occasional progress feedback when something takes a while.
Views:
image pull on create:
image push on deploy (lowered interval for screenshot because local pushes are fast):
package pull:
package publish:
Related Issue
Fixes #3633
Checklist before merging