cmd/snap: improve snap size reporting#16669
cmd/snap: improve snap size reporting#16669bboozzoo wants to merge 3 commits intocanonical:masterfrom
Conversation
github-actions
bot
added
the
Needs Documentation -auto-
bboozzoo
force-pushed
the
bboozzoo/fix-quantity-snap-info
branch
from
29babaa to
198bab2
Compare
The code used a helper which rounds down the quantities for formatting. The rounding error isn't super significant while in the MBs range, but above 1GB, there's an observable difference between 1.9GB and 1GB. Switch to a different helper which tries to preserve the informational value of the formatted quantity. Fixes: LP#2142655 Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
bboozzoo
force-pushed
the
bboozzoo/fix-quantity-snap-info
branch
from
198bab2 to
69d020f
Compare
|
Fri Feb 27 20:43:18 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skip
|
There was a problem hiding this comment.
Pull request overview
This PR improves snap size reporting in the snap info command by switching from strutil.SizeToStr (which rounds down) to quantity.FormatAmount (which preserves informational value with decimal precision). This addresses the issue where sizes above 1GB would lose significant precision, e.g., 1.9GB displaying as 1GB.
Changes:
- Replaced
strutil.SizeToStrwithquantity.FormatAmount(uint64(size), -1) + "B"in channel info formatting - Added comprehensive test coverage for large sizes (GB and MB ranges) with new test
TestInfoHumanSizes - Updated all existing test expectations to reflect the new, more precise formatting
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/snap/cmd_info.go | Changed size formatting from strutil.SizeToStr to quantity.FormatAmount for better precision |
| cmd/snap/cmd_info_test.go | Updated test expectations for new size format and added new test for large sizes |
| revision: fmt.Sprintf("(%s)", revision), | ||
| size: strutil.SizeToStr(size), | ||
| notes: notes.String(), | ||
| // -1 for auto-width of 5 chars |
There was a problem hiding this comment.
For consistency, consider also updating the size formatting in cmd/snap/cmd_snap_op.go line 1123 (snap refresh --list command) and cmd/snap/cmd_component.go line 104 (snap component command) to use the same quantity.FormatAmount approach. This would ensure all snap size displays benefit from the improved precision, especially for sizes above 1GB.
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
| esc *escapes | ||
| } | ||
|
|
||
| func formatSize(sz int64) string { |
There was a problem hiding this comment.
The conversion from int64 to uint64 in formatSize is potentially unsafe if negative size values occur. While the current codebase doesn't appear to produce negative sizes in practice, the type conversion would wrap negative values to very large positive values (e.g., -1 becomes 18446744073709551615).
Consider adding a guard to handle negative values explicitly, for example:
func formatSize(sz int64) string {
if sz < 0 {
return "n/a"
}
return quantity.FormatAmount(uint64(sz), -1) + "B"
}This would make the function more defensive and prevent confusing output if negative sizes are ever introduced through bugs or API changes.
| func formatSize(sz int64) string { | |
| func formatSize(sz int64) string { | |
| if sz < 0 { | |
| return "n/a" | |
| } |
ZeyadYasser
left a comment
ZeyadYasser
left a comment
There was a problem hiding this comment.
LGTM, Thank you!
Would it make sense to remove strutil.SizeToStr in a follow up?
Mohit-Chachada
left a comment
Mohit-Chachada
left a comment
There was a problem hiding this comment.
Thanks! Just a few minor inline comments.
Additionally, I saw that there is one more usage of strutil.SizeToStr remaining at https://github.com/canonical/snapd/blob/master/osutil/disk.go#L37. Perhaps, we want to change that too and delete strutil.SizeToStr altogether? Removing it would help to prevent any such future (mis-)uses of it.
| esc *escapes | ||
| } | ||
|
|
||
| func formatSize(sz int64) string { |
There was a problem hiding this comment.
this already exists (with a different name) at https://github.com/canonical/snapd/blob/master/cmd/snap/cmd_snapshot.go#L36-L38.
Could you reuse that?
There was a problem hiding this comment.
I see both are the same function but with different name
There was a problem hiding this comment.
but not sure if makes sense to move to strutil/quantity.
| revision: fmt.Sprintf("(%s)", revision), | ||
| size: strutil.SizeToStr(size), | ||
| notes: notes.String(), | ||
| // -1 for auto-width of 5 chars |
There was a problem hiding this comment.
minor: the comment seems out of place as there is no literal -1 here. Maybe it was from before when the actual call to quantity.FormatAmount with -1 as argument was here.
The comment suits more inside the formatSize method but as mentioned above we could instead re-use the already existing method from cmd_snapshot.go. If helpful, we can add this comment there.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #16669 +/- ##
==========================================
+ Coverage 77.52% 77.53% +0.01%
==========================================
Files 1355 1359 +4
Lines 186844 187316 +472
Branches 2449 2446 -3
==========================================
+ Hits 144851 145240 +389
- Misses 33237 33299 +62
- Partials 8756 8777 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The code used a helper which rounds down the quantities for formatting. The rounding error isn't super significant while in the MBs range, but above 1GB, there's an observable difference between 1.9GB and 1GB.
Switch to a different helper which tries to preserve the informational value of the formatted quantity.
Fixes: LP#2142655 SNAPDENG-36492
Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?