Fix #26 - RAMInBytes Bug #27
Conversation
RAMInBytes wasn't able to parse the output of BytesSize until now. Signed-off-by: Florin Asavoaie <FlorinAsavoaie@users.noreply.github.com>
|
@thaJeztah Not sure who can look at this, pinging you so that someone is aware about the PR. |
| @@ -116,6 +116,8 @@ func TestRAMInBytes(t *testing.T) { | |||
| assertSuccessEquals(t, 32*KiB, RAMInBytes, "32K") | |||
| assertSuccessEquals(t, 32*KiB, RAMInBytes, "32kb") | |||
| assertSuccessEquals(t, 32*KiB, RAMInBytes, "32Kb") | |||
| assertSuccessEquals(t, 32*KiB, RAMInBytes, "32Kib") | |||
| assertSuccessEquals(t, 32*KiB, RAMInBytes, "32KIB") | |||
There was a problem hiding this comment.
Looking at these, I wonder if it's correct that we treat kB, KiB and KB equal; from https://en.wikipedia.org/wiki/Kilobyte
32 KB == 32 KiB == 32768 byte, but 32kB == 32000 byte
There was a problem hiding this comment.
Actually, wondering if I'm reading KB (JEDEC) correctly in that table, so please double-check 😅
There was a problem hiding this comment.
Well, that was, obviously, already there. What I added should be the correct behavior imho. However, I wonder how many things will break if we decide to mess with it.
On the other hand, there's no function here to make a difference between KB and KiB, it just depends on what function does the user of the library call.
There was a problem hiding this comment.
Yes, I noticed it was there 😞 not sure what's best; perhaps we need additional functions
There was a problem hiding this comment.
Well, I don't think this module is organized in such a way to properly support the standards. I'd rather go for a rebuild of this module (go-units2 or go-standard-units :D) and organize the functions something like ReadHumanSizeBinary which properly parses IEC and JEDEC, then ReadHumanSizeDecimal which properly parses metric data. Also organize those maps and regexes per "Metric/Decimal" and "Binary/IEC/JEDEC".
My guess is that it would break a lot of stuff because these conventions are badly used all around.
Until then, would you think this is a blocker to merge this?
There was a problem hiding this comment.
Agreed, this change doesn't make things worse, so don't see a reason to not go ahead.
Let me know if you're interested in a follow up for those things though!
relevant changes: - docker/go-units#19 make 1 second not to be plural seconds - docker/go-units#20 Add `HumanSizeWithPrecision` function - docker/go-units#21 change week display rule - docker/go-units#22 Better human duration precision - docker/go-units#23 Removes spaces before unit - docker/go-units#27 Fix containerd#26 - RAMInBytes Bug - docker/go-units#33 Fix handling of unlimited (-1) ulimit values - docker/go-units#34 Revert 46 minute threshold Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's not exactly a nitpick. debos uses github.com/docker/go-units to parse the size, and docker/go-units is fundamentally broken, as it parses both GiB anb GB as GB. It's a known issue that's been there forever, and will probably never be fixed because backward compat (that's my guess). Cf. <docker/go-units#31>, or better <docker/go-units#27 (comment)>. So let's make sure that we talk about GB, because that's what happens in practice.
It's not exactly a nitpick. debos uses github.com/docker/go-units to parse the size, and docker/go-units is fundamentally broken, as it parses both GiB anb GB as GB. It's a known issue that's been there forever, and will probably never be fixed because backward compat (that's my guess). Cf. <docker/go-units#31>, or better <docker/go-units#27 (comment)>. So let's make sure that we talk about GB, because that's what happens in practice.
RAMInBytes wasn't able to parse the output of BytesSize until now.
Signed-off-by: Florin Asavoaie FlorinAsavoaie@users.noreply.github.com