fix(encode): ensure quoting is maintained for scalars #115
Conversation
Currently, when encoding or decoding integers bigger than 64 bytes using `sigs.k8s.io/yaml/goyaml.v3`, those values are interpreted as unequivocal strings, which causes quotes to be removed. However, those values should be treated as regular numbers since they conform to the integer format in the YAML specification and honour quoting when present in the original input.
k8s-ci-robot
added
the
sig/api-machinery
k8s-ci-robot
added
cncf-cla: yes
| A4: 0x0000000000000000000000010000000000000000 | ||
| A5: 0x000000000000000000000000FFFFFFFFFFFFFFFF |
There was a problem hiding this comment.
This repo is oriented at yaml which can be round-tripped to json and used in Kubernetes API objects
These bigints would be problematic for Kubernetes as they do not round-trip to json ... I'm on the fence that we should enable this in this library
There was a problem hiding this comment.
My general thought process was that unquoted values are already not round-trippable to JSON when bigger than 64 bytes, but if quoted those should be maintained. The current implementation assumes that if an integer value does not fit a 64-byte integer then it must be a string but that's not exactly true.
| A1: "0x0000000000000000000000010000000000000000" | ||
| A2: "0x000000000000000000000000FFFFFFFFFFFFFFFF" |
There was a problem hiding this comment.
maintaining the quoting on A1/A2 is good ... do we also know that YAMLToJSON works properly here? that's what will be used when reading a yaml manifest and sending it to the kube-apiserver
There was a problem hiding this comment.
I'll add a test case for YAMLToJSON
There was a problem hiding this comment.
understanding all the paths that come through here and what they do with these values today (as well as how kube-apiserver treats submitted yaml values) is important so we make sure we don't break anyone that is currently "working"
I think the test cases would be something like:
unquoted yaml → yaml (round-trip)
unquoted yaml → json (conversion)
quoted yaml → yaml (round-trip)
quoted yaml → json (conversion)
There was a problem hiding this comment.
|
This does not fix #108 |
Add round-trip tests as requested in code review and fix a few other scenarios uncovered by the addition of new tests.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stormqueen1990 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/L
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
Currently, when encoding or decoding integers bigger than 64 bytes using
sigs.k8s.io/yaml/goyaml.v3, those values are interpreted as unequivocal strings, which causes quotes to be removed. However, those values should be treated as regular numbers since they conform to the integer format in the YAML specification and honour quoting when present in the original input.Related to kubernetes-sigs/kustomize#5432 and kubernetes-sigs/kustomize#5558
Fixes #108