feat: Add volume usage progress bar - #423
Conversation
f799cd3 to
4be53be
Compare
| emptyColor := ansi.NewStyle().ForegroundColor(ansi.IndexedColor(238)).String() | ||
| reset := ansi.NewStyle().ForegroundColor(nil).String() | ||
|
|
||
| dots := []rune{'⣀', '⣄', '⣤', '⣦', '⣶', '⣷', '⣿'} |
There was a problem hiding this comment.
I wonder if this format can be found somewhere in bubbletea or smth else? I've seen it before
There was a problem hiding this comment.
Pull request overview
Adds colored volume-usage sparklines to volume listings.
Changes:
- Introduces a generic sparkline usage type.
- Derives volume usage from size and free capacity.
- Updates volume output and help golden files.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
internal/types/quota.go |
Implements usage ratio and sparkline rendering. |
internal/cmd/volumes.go |
Adds and populates the volume usage field. |
internal/cmd/testdata/TestOutput/volumes |
Updates expected volume output. |
cmd/unikraft/testdata/TestHelp/volumes |
Adds usage to documented fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Added a custom compare for custom types like sparkling line. It would be nice to support >,<,>=,<= operators but I seen that it already have an issue |
1b17f34 to
da055c5
Compare
Signed-off-by: ayn <ayn.khatib@gmail.com>
a729717 to
00346ab
Compare
|
@khatibomar can you close the copilot comments that you resolved/ignored so they don't get in the way 🙏 |
craciunoiuc
left a comment
craciunoiuc
left a comment
There was a problem hiding this comment.
Some sanity comments my side. Feel free to comment or ignore
I think it looks fine already 🙏
I think after we can get @jedevc also in here for a final opinion
| case ratio <= 0.5: | ||
| color = ansi.NewStyle().ForegroundColor(colors.Success).String() | ||
| case ratio <= 0.8: | ||
| color = ansi.NewStyle().ForegroundColor(colors.Warning).String() | ||
| default: | ||
| color = ansi.NewStyle().ForegroundColor(colors.Error).String() | ||
| } |
There was a problem hiding this comment.
Just checking, did you compare these colors with the quota bars in the quota command? Same for the percentages where these change
There was a problem hiding this comment.
I think the quota have only a blue bar, it doesn't have a color based on percentage.
There was a problem hiding this comment.
Ah sorry, it means we removed that in the end, all good I think, but I'll tag maybe also @jedevc
Do we want percentage colouring or do we remove colors here also?
There was a problem hiding this comment.
Hm, we should have quotas coloring 🎉
| const width = 10 | ||
|
|
There was a problem hiding this comment.
could the width be easily made variable? Aka, min 5, max 10 width, like the quotas
There was a problem hiding this comment.
if you mean metro_quotas.go they are set to be depending on the screen size, I think same thing can be done in here and limited between [5,10] range
There was a problem hiding this comment.
Yes exactly, I'm thinking if it would make sense here also to make it variable, so smaller terminals also work
| state: available | ||
| size: 50MiB | ||
| free: 10MiB | ||
| usage: ⣿⣿⣿⣿⣿⣿⣿⣿⣀⣀ 40MiB/50MiB |
There was a problem hiding this comment.
nit: I wonder if maybe we should show the number first, then the number first?
| usage: ⣿⣿⣿⣿⣿⣿⣿⣿⣀⣀ 40MiB/50MiB | |
| usage: 40MiB/50MiB ⣿⣿⣿⣿⣿⣿⣿⣿⣀⣀ |
Or maybe even as a percentage?
| usage: ⣿⣿⣿⣿⣿⣿⣿⣿⣀⣀ 40MiB/50MiB | |
| usage: 80% ⣿⣿⣿⣿⣿⣿⣿⣿⣀⣀ |
There was a problem hiding this comment.
hmmm prefer other way - since the bar is fixed width and looks more aligned in a table
There was a problem hiding this comment.
Fair point. We could always " " pad it if it was a percentage.
I think I more care about the percentage than the side it's on, though I would argue that having the most significant info (the number) first is useful.
| state: available | ||
| size: 50MiB | ||
| free: 10MiB | ||
| usage: ⣿⣿⣿⣿⣿⣿⣿⣿⣀⣀ 40MiB/50MiB |
There was a problem hiding this comment.
❯ unikraft volume inspect foo -oquiet -f usage
⣀⣀⣀⣀⣀⣀⣀⣀⣀⣀ 0B/10MiB
Quiet mode should show the direct underlying value, ideally as a percentage, see above.
| // Comparer is an interface for types that provide a custom comparable value. | ||
| type Comparer interface { | ||
| CompareValue() any | ||
| } |
There was a problem hiding this comment.
I don't think we should do this interface. Things should implement cmp.Compare to do the comparison.
Somewhere else I had the idea that we should have a custom Render implementation for rendering to the screen, but for filtering, we would just use .String() or .MarshalText or whatever instead of doing the weird post-ASCII filtering.
| func (u SparklineUsage[T]) String() string { | ||
| if u.Total == 0 { | ||
| return "N/A" | ||
| } |
There was a problem hiding this comment.
Essentially, as mention above, this method would just become Render, while String would be implemented to be just the raw percentage.
This would be a bit of a refactor in a few other places, but the code would then feel much neater I think 🎉
| case ratio <= 0.5: | ||
| color = ansi.NewStyle().ForegroundColor(colors.Success).String() | ||
| case ratio <= 0.8: | ||
| color = ansi.NewStyle().ForegroundColor(colors.Warning).String() | ||
| default: | ||
| color = ansi.NewStyle().ForegroundColor(colors.Error).String() | ||
| } |
There was a problem hiding this comment.
Hm, we should have quotas coloring 🎉
|
What do you mean quotas coloring you told me to remove the colors some time ago 😭 😭 😭 😭 |
I seem to remember suggesting making the color bars have reasonable boundaries, there were some very magic numbers in there as I remember it 🤔 #337 (comment) I don't remember suggesting removing them entirely, but maybe I did - if so, woops. |
|
Ah, maybe I got it wrong, eh too late now :D |
Closes #373