goldilocks icon indicating copy to clipboard operation
goldilocks copied to clipboard

Support Mi units

Open jetersen opened this issue 1 year ago • 4 comments

This PR fixes #522

Checklist

  • [x] I have signed the CLA
  • [ ] I have updated/added any relevant documentation

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 https://github.com/FairwindsOps/goldilocks/issues/522#issuecomment-1793018418

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:

jetersen avatar Nov 03 '23 22:11 jetersen

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 03 '23 22:11 CLAassistant

Not sure if it helpful to add things to the docs about the command flag for the dashboard? Perhaps in the helm chart?

jetersen avatar Nov 03 '23 22:11 jetersen

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 M with Mi to avoid the Quantity math 🤔 Perhaps there is another go api or library to do rounding using BinarySI units?

jetersen avatar Nov 03 '23 22:11 jetersen

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 avatar Dec 18 '23 19:12 sudermanjr

@sudermanjr @transient1 This definitely not stale 😓

jetersen avatar Feb 28 '24 06:02 jetersen