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