-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(encode): ensure quoting is maintained for scalars #115
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ import ( | |
"os" | ||
|
||
. "gopkg.in/check.v1" | ||
"sigs.k8s.io/yaml/goyaml.v3" | ||
yaml "sigs.k8s.io/yaml/goyaml.v3" | ||
) | ||
|
||
var marshalIntTest = 123 | ||
|
@@ -731,6 +731,40 @@ func (s *S) TestSortedOutput(c *C) { | |
} | ||
} | ||
|
||
func (s *S) TestQuotingMaintained(c *C) { | ||
var buf bytes.Buffer | ||
var yamlValue map[string]interface{} | ||
const originalYaml = `data: | ||
A1: "0x0000000000000000000000010000000000000000" | ||
A2: "0x000000000000000000000000FFFFFFFFFFFFFFFF" | ||
A3: "1234" | ||
A4: 0x0000000000000000000000010000000000000000 | ||
A5: 0x000000000000000000000000FFFFFFFFFFFFFFFF | ||
Comment on lines
+741
to
+742
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
A6: 1234 | ||
` | ||
const outputYaml = `data: | ||
A1: "0x0000000000000000000000010000000000000000" | ||
A2: "0x000000000000000000000000FFFFFFFFFFFFFFFF" | ||
A3: "1234" | ||
A4: 18446744073709551616 | ||
A5: 18446744073709551615 | ||
A6: 1234 | ||
` | ||
|
||
dec := yaml.NewDecoder(strings.NewReader(originalYaml)) | ||
errDec := dec.Decode(&yamlValue) | ||
c.Assert(errDec, IsNil) | ||
|
||
enc := yaml.NewEncoder(&buf) | ||
errEnc := enc.Encode(yamlValue) | ||
c.Assert(errEnc, IsNil) | ||
|
||
errClose := enc.Close() | ||
c.Assert(errClose, IsNil) | ||
|
||
c.Assert(buf.String(), Equals, outputYaml) | ||
} | ||
|
||
func newTime(t time.Time) *time.Time { | ||
return &t | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a test case for YAMLToJSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt I added some test cases to the
TestYAMLtoJSON
function inyaml_test.go
in my last commit 742f5e6, which I believe covers both of these scenarios since it first converts the YAML to JSON, and then converts it back, but please let me know if I misunderstood!