add support for nanocpus (fixes issue #18) #19
Conversation
There was a problem hiding this comment.
Thanks for this PR.
Could you please add unit tests to the unit parsing and arithmetics with different scales?
Funnily enough, I initially derived the supported suffixes from the code comment in the apimachinery repo, yet that comment does not include the "u" and "n" scales.
Would you also add the "u" one, including tests? (see: https://github.com/kubernetes/apimachinery/blob/master/pkg/api/resource/suffix.go line 94)
philgebhardt
force-pushed
the
pag/support-nanocpus
branch
from
9d9cc66 to
7e32d85
Compare
philgebhardt
force-pushed
the
pag/support-nanocpus
branch
from
7e32d85 to
079550d
Compare
| Scale::Nano => "n".to_owned(), | ||
| Scale::Micro => "u".to_owned(), |
There was a problem hiding this comment.
Ah, like Milli, I suppose these should be "", given they aren't applicable?
There was a problem hiding this comment.
That's a good question.
I'll have to look into the kube source and how they are handling it.
In the current implementation in this crate, I assumed that Milli would be the smallest unit, and then I "upgraded" it to a simple decimal. However, considering there are smaller denominations, this might need to be changed, and the smaller units should be returned. (Or add modulo logic so that if the value modulo 1000 is 0, the next higher unit is used, and the number is divided by 1000).
There was a problem hiding this comment.
But this should not block this PR from getting merged, as from a mathematical point of view, the results are correct.
|
Hey, sorry for the delay. I was out due to a prolonged health issue. I am taking a look now. |
|
Thanks for this PR, @philgebhardt 👍 I'll soon cut a release, as I want to fix multiplication before including it in a release (currently, multiplication does not multiply the factors together). |
fixes: #18