Support Mi units #667
Support Mi units #667
Conversation
|
|
| basePath string | ||
| insightsHost string | ||
| enableCost bool | ||
| useMemoryBinarySI bool |
| @@ -43,6 +44,7 @@ func init() { | |||
| dashboardCmd.PersistentFlags().StringVar(&basePath, "base-path", "/", "Path on which the dashboard is served.") | |||
| dashboardCmd.PersistentFlags().BoolVar(&enableCost, "enable-cost", true, "If set to false, the cost integration will be disabled on the dashboard.") | |||
| dashboardCmd.PersistentFlags().StringVar(&insightsHost, "insights-host", "https://insights.fairwinds.com", "Insights host for retrieving optional cost data.") | |||
| dashboardCmd.PersistentFlags().BoolVar(&useMemoryBinarySI, "binary-si", false, "Use binary SI units for memory (base 2) instead of decimal SI units (base 10).") | |||
There was a problem hiding this comment.
Suggestion please, naming is hard 😓
| for len(mem.String()) > maxAllowableStringLen && i < len(memoryScales)-1 { | ||
| maxLength := 5 | ||
| if len(mem.String()) <= maxLength { | ||
| return rl |
There was a problem hiding this comment.
Since RoundUp is the issue in this case I decided to simply return early here 🤔
| case resource.Kilo: | ||
| scaleValue = Kibibyte | ||
| case resource.Mega: | ||
| scaleValue = Mebibyte | ||
| case resource.Giga: | ||
| scaleValue = Gibibyte | ||
| case resource.Tera: | ||
| scaleValue = Tebibyte |
There was a problem hiding this comment.
Followed the scaled used below
resource.Kilo,
resource.Mega,
resource.Giga,
resource.Tera,
|
Not sure if it helpful to add things to the docs about the command flag for the dashboard? Perhaps in the helm chart? |
|
The question becomes does this even need to be a command flag option. Since most tooling uses Mi and I wonder if we should simply replace the different units like |
|
Thanks for the PR! We will likely need some extra time to review this - it's an issue that has plagued us for a while :-D I do like making this a flagged feature to start, even if we plan to default differently later. |
|
@sudermanjr @transient1 This definitely not stale 😓 |
This PR fixes #522
Checklist
Description
What's the goal of this PR?
Add support for using BinarySI since due to the use of RoundUp converts the Memory units to DecimalSI
What changes did you make?
The main issue is RoundUp which converts the units to DecimalSI to do the rounding.
So I created a basic method that uses math to convert the unit back to BinarySI although due to the difference in the units there is a slight difference like 124M turns into 119Mi.
Perhaps a different approach is to simply replace K,M,G,T with Ki, Mi, Gi, Ti 🤔
What alternative solution should we consider, if any?
See #522 (comment)
Perhaps there is a different approach to rounding 🤔
I tried other solutions but did not get the desired result from String method 😓
This applies to this PR:
