Skip to content

feat: add default output logs to create #63

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.

Sign up for GitHub

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

Open
arortiz-rh wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add default output logs to create #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions cmd/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
var (
createVerbose bool
createDebug bool
createQuiet bool
createParallelism int
createSignatures []string
)
Expand All @@ -37,6 +38,12 @@ func init() {
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")

@knecasov knecasov Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

@knecasov knecasov Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just curious - is there a plan/ticket/issue to add tests for bytesToMB function and quiet/debug/verbose flag interaction, please?

@arortiz-rh arortiz-rh Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add tests for bytesToMB as part of this PR

return fmt.Sprintf("%.2f MB", float64(b)/(1024*1024))

@knecasov knecasov Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytesToMB divides by 1024*1024 but labels the result MB. Either use 1000*1000 for MB or label it MiB.

}

func runCreate(cmd *cobra.Command, args []string) error {
Expand All @@ -46,7 +53,7 @@ func runCreate(cmd *cobra.Command, args []string) error {
}
defer os.RemoveAll(tmpDir)

log := &cmdLogger{debug: createDebug}
log := &cmdLogger{debug: createDebug, quiet: createQuiet}

log.Debug("Opening old image: %s", args[0])

Expand All @@ -66,6 +73,10 @@ func runCreate(cmd *cobra.Command, args []string) error {

sigReaders := ocidelta.ExtractedSignatures(newReader)

if len(createSignatures) > 0 {
log.Default("Embedding %d signature(s)", len(createSignatures))
}

for _, sigPath := range createSignatures {
log.Debug("Opening signature: %s", sigPath)

Expand Down Expand Up @@ -96,6 +107,14 @@ func runCreate(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to write output: %w", err)
}

if !createVerbose && stats.ProcessedLayerBytes > 0 {

@knecasov knecasov Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new default output block dereferences stats without a nil check. I would add stats != nil.

saved := stats.ProcessedLayerBytes - stats.TarDiffLayerBytes - stats.OriginalLayerBytes
log.Default("\nDelta complete: %d layer(s) diffed, %d reused, bytes saved %.2f%% (%s → %s)", stats.ProcessedLayers, stats.SkippedLayers,
float64(saved)/float64(stats.ProcessedLayerBytes)*100, bytesToMB(stats.ProcessedLayerBytes), bytesToMB(stats.ProcessedLayerBytes-saved))
} else if !createVerbose {
log.Default("\nDelta complete: %d layer(s) diffed, %d reused", stats.ProcessedLayers, stats.SkippedLayers)
}

if createVerbose && stats != nil {

@knecasov knecasov Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--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.

@arortiz-rh arortiz-rh Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fmt.Printf("\nDelta creation statistics:\n")
fmt.Printf(" Old image layers: %d\n", stats.OldLayers)
Expand All @@ -109,9 +128,11 @@ func runCreate(cmd *cobra.Command, args []string) error {
if stats.ProcessedLayerBytes > 0 {
saved := stats.ProcessedLayerBytes - stats.TarDiffLayerBytes - stats.OriginalLayerBytes
pct := float64(saved) / float64(stats.ProcessedLayerBytes) * 100
fmt.Printf(" Bytes saved: %d (%.1f%%)\n", saved, pct)
fmt.Printf(" Bytes saved: %d (%.1f%%)\n\n", saved, pct)
}
}

log.Default("Delta written to %s", args[2])

return nil
}
9 changes: 9 additions & 0 deletions cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,20 @@ func Root() *cobra.Command {
type Logger interface {
Debug(format string, args ...interface{})
Warning(format string, args ...interface{})
Default(format string, args ...interface{})
}

// cmdLogger implements the Logger interface
type cmdLogger struct {
debug bool
quiet bool
}

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.

@knecasov knecasov Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@arortiz-rh arortiz-rh Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if !l.quiet && !l.debug {
fmt.Printf(format+"\n", args...)
}
}

func (l *cmdLogger) Debug(format string, args ...interface{}) {
Expand Down
2 changes: 2 additions & 0 deletions pkg/oci-delta/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ import (

type Logger interface {
Debug(format string, args ...interface{})
Default(format string, args ...interface{})
Warning(format string, args ...interface{})
}

type SilentLogger struct{}

func (SilentLogger) Debug(string, ...interface{}) {}
func (SilentLogger) Default(string, ...interface{}) {}
func (SilentLogger) Warning(string, ...interface{}) {}

var ociLayoutFileData = []byte(`{"imageLayoutVersion":"1.0.0"}`)
Expand Down
6 changes: 6 additions & 0 deletions pkg/oci-delta/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,19 @@ func CreateDelta(oldReader OCIReader, newReader OCIReader, writer OCIWriter, opt
log.Debug("Layers with new content (will process): %d", stats.ProcessedLayers)
log.Debug("Layers with existing content (will skip): %d", stats.SkippedLayers)

log.Default("Found %d layer(s) to diff, %d layer(s) unchanged", stats.ProcessedLayers, stats.SkippedLayers)

log.Debug("\nProcessing layers...")
log.Default("\nProcessing layers...")

for _, l := range new.layers {
if !newOnlyLayers[l.Digest] {
log.Debug(" Skipping layer with existing content %s", l.Digest.Encoded()[:16])
}
}

log.Default(" Skipped %d layer(s) with existing content", stats.SkippedLayers)

layerResults, err := computeLayerDiffsParallel(log, old, new, newOnlyLayers, opts.TmpDir, opts.Parallelism)
if err != nil {
return nil, err
Expand Down Expand Up @@ -407,6 +412,7 @@ func computeLayerDiff(log Logger, old *OCIImage, new *OCIImage, blobDigest diges
}
sizeReader.Close()

log.Default(" Computing diff for layer %d/%d", layerNum, total)
log.Debug(" Computing diff for layer %d/%d %s (%d bytes)", layerNum, total, blobDigest.Encoded()[:16], originalSize)

tmpFile, err := os.CreateTemp(tmpDir, "oci-delta-*.tar-diff")
Expand Down
1 change: 1 addition & 0 deletions pkg/oci-delta/verify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
type testLogger struct{}

func (l *testLogger) Debug(format string, args ...interface{}) {}
func (l *testLogger) Default(format string, args ...interface{}) {}
func (l *testLogger) Warning(format string, args ...interface{}) {}

func createTestKey(t *testing.T) (sigstoreSignature.Verifier, *ecdsa.PrivateKey) {
Expand Down