Add support for k8s bytes suffices #53
Conversation
There was a problem hiding this comment.
Looks like there is a lint failure - may need to refactor the code some to reduce complexity.
I'm not sure if we want to support these units on all field types. It looks like parseUintLiteral is called via parseIntLiteral which is in turn used to unmarshal enum and float values. I'm wondering if we don't support this behavior only on field names that end in _bytes or at least restrict to non-enum types that are represented with int32/int64/uint32/uint64.
| if err != nil { | ||
| parsedFloat, floatErr := strconv.ParseFloat(value, 64) | ||
| if floatErr != nil || parsedFloat < 0 || math.IsInf(parsedFloat, 0) || math.IsNaN(parsedFloat) { | ||
| if err == nil || base != 10 { |
There was a problem hiding this comment.
Don't we want to support hex literals as well if this fails? https://go.dev/ref/spec#Floating-point_literals
|
|
||
| func parseNumberWithUnit(str string) (whole, frac uint64, scale *big.Int, unitName string, rem string, err error) { | ||
| // The next character must be [0-9.] | ||
| if !(str[0] == '.' || '0' <= str[0] && str[0] <= '9') { |
There was a problem hiding this comment.
I believe we'll panic here on an empty string - verified by adding this to the test suite:
- single_fixed64: ""|
|
||
| // A map from unit to the number of bytes in that unit. | ||
| var bytesPerUnitMap = map[string]int64{ | ||
| "K": 1000, |
There was a problem hiding this comment.
| "K": 1000, | |
| "k": 1000, |
https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/quantity/#Quantity
| "Pi": 1 << 50, | ||
| "E": 1000 * 1000 * 1000 * 1000 * 1000 * 1000, | ||
| "Ei": 1 << 60, | ||
| } |
There was a problem hiding this comment.
k8s also supports m (milli): https://github.com/kubernetes/kubernetes/blob/ed9572d9c7733602de43979caf886fd4092a7b0f/staging/src/k8s.io/apimachinery/pkg/api/resource/suffix.go#L96-L97
This doesn't make sense for bytes but if we want to support CPU units this could be useful.
| // bytesUnitNames is the list of byte unit names. | ||
| // E is exabyte, P is petabyte, T is terabyte, G is gigabyte, M is megabyte, K is kilobyte. | ||
| // The i suffix indicates a binary unit (e.g., Ki = 1024). | ||
| var bytesUnitNames = []string{"K", "M", "G", "T", "P", "E", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei"} |
There was a problem hiding this comment.
| var bytesUnitNames = []string{"K", "M", "G", "T", "P", "E", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei"} | |
| var bytesUnitNames = []string{"k", "M", "G", "T", "P", "E", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei"} |
| return whole, frac, scale, unitName, str, errors.New("invalid number, expected digit") | ||
| } | ||
| var pre bool // Whether we have seen a digit before the dot. | ||
| whole, str, pre, err = leadingInt(str) |
There was a problem hiding this comment.
Do we want to enforce a max length on the str somewhere so we don't read arbitrarily large bigint values and perform computations on them?
Documented here: https://kubernetes.io/docs/tasks/configure-pod-container/assign-memory-resource/#memory-units