-
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
bib: integrate upload progress into main progressbar #763
base: main
Are you sure you want to change the base?
Conversation
This commit adds a new `Clear()` method on the progressBar so that we can clear subprogress easily. This is useful when switching from image building to image uploading, here we need a single progress again.
This commit improves the upload progress by integrating it with the new overall progress flow. After the building is done the upload will be part of the overall progress bar flow.
22b4adb
to
060ddbc
Compare
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.
Looks good! Just one request regarding AMI registration, and also linter isn't happy.
file, err := os.Open(filename) | ||
if err != nil { | ||
return fmt.Errorf("cannot upload: %v", err) | ||
} | ||
defer file.Close() | ||
|
||
keyName := fmt.Sprintf("%s-%s", uuid.New().String(), filepath.Base(filename)) | ||
fmt.Fprintf(osStdout, "Uploading %s to %s:%s\n", filename, bucketName, keyName) | ||
pbar.SetMessagef("Uploading %s to %s:%s\n", filename, bucketName, keyName) |
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.
Can we have a separate message for the registering step? See lines 76-79. Note that the AMI id should be printed to stdout, so people can use bib in scripts. Other messages probably don't belong to stdout.
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
Build on top of #743