feat: add default output logs to create#63
Conversation
- Default output shows progress & simple stats about the delta created - Added quiet flag to supress default output if needed Fixes containers#43 Signed-off-by: Armando Ortiz <arortiz@redhat.com>
|
|
||
| // bytesToMB formats a byte count as a human-readable megabyte value. Used in default output for simpler feedback. | ||
| func bytesToMB(b int64) string { | ||
| return fmt.Sprintf("%.2f MB", float64(b)/(1024*1024)) |
There was a problem hiding this comment.
bytesToMB divides by 1024*1024 but labels the result MB. Either use 1000*1000 for MB or label it MiB.
| log.Default("\nDelta complete: %d layer(s) diffed, %d reused", stats.ProcessedLayers, stats.SkippedLayers) | ||
| } | ||
|
|
||
| if createVerbose && stats != nil { |
There was a problem hiding this comment.
--quiet and --verbose are not mutually exclusive - --quiet --verbose would still print verbose output since the verbose block uses fmt.Printf directly. It is worth either adding a !createQuiet guard to the verbose block or using MarkFlagsMutuallyExclusive.
There was a problem hiding this comment.
I didn't have the quiet flag affect the verbose flag's output purposefully so that the user could obtain the stats if they really wanted to without any progress information, such as if they're piping the output to a file. It's a niche use case, but I would rather include that functionality since I don't see any other reason why a user would attempt to use --quiet and --verbose at the same time.
If you still think it would be good change that, I can implement it. I just wanted to explain that reasoning before I made that change
| return fmt.Errorf("failed to write output: %w", err) | ||
| } | ||
|
|
||
| if !createVerbose && stats.ProcessedLayerBytes > 0 { |
There was a problem hiding this comment.
The new default output block dereferences stats without a nil check. I would add stats != nil.
| } | ||
|
|
||
| func (l *cmdLogger) Default(format string, args ...interface{}) { | ||
| // Debug is a much more detailed version of the default logs, no need for both to exist at the same time. |
There was a problem hiding this comment.
The Default suppression in debug mode makes sense per the comment, but "Delta written to %s" does not have a log.Debug equivalent - so this message is lost entirely in debug mode. Is that intentional, please?
There was a problem hiding this comment.
It was a simple comment I thought to add to the default logs initially, but you're right there is no debug equivalent. I will add a line to the log.Debug output that shares the name and file path it was written to, since I think that's good information to have.
| createCmd.Flags().BoolVar(&createDebug, "debug", false, "show detailed progress information") | ||
| createCmd.Flags().IntVarP(&createParallelism, "jobs", "j", 0, "max parallel tar-diff workers (default: number of CPUs)") | ||
| createCmd.Flags().StringArrayVar(&createSignatures, "signature", nil, "signature OCI artifact to embed (can be specified multiple times)") | ||
| createCmd.Flags().BoolVarP(&createQuiet, "quiet", "q", false, "silences all default output") |
There was a problem hiding this comment.
I would write "suppress all default output" to match the imperative style of the other flags.
| } | ||
|
|
||
| // bytesToMB formats a byte count as a human-readable megabyte value. Used in default output for simpler feedback. | ||
| func bytesToMB(b int64) string { |
There was a problem hiding this comment.
I am just curious - is there a plan/ticket/issue to add tests for bytesToMB function and quiet/debug/verbose flag interaction, please?
There was a problem hiding this comment.
I can add tests for bytesToMB as part of this PR
Added a default output to the
createoption when making a delta.I included some examples of how the default output now looks compared to the other options to show how it's different than what is currently available. There is also a
--quiet/-qflag that suppresses all the default output.Example Output (New Default)
Example Output (Verbose)
Example Output (Debug)
Fixes #43