-
Notifications
You must be signed in to change notification settings - Fork 557
fix(yaml): add support for YAML 1.1 parsing in Kubernetes manifests #2660
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -154,4 +154,55 @@ spec: | |||||
// not using strict equality as types are not matching | ||||||
deepEqual(actual, expected); | ||||||
}); | ||||||
|
||||||
it('should parse octal values correctly using default YAML 1.1', () => { | ||||||
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.
Suggested change
|
||||||
const yamlStr = ` | ||||||
apiVersion: v1 | ||||||
kind: ConfigMap | ||||||
metadata: | ||||||
name: test | ||||||
data: | ||||||
mode: 0644 | ||||||
`; | ||||||
const obj = loadYaml<{ | ||||||
data: { mode: number }; | ||||||
metadata: { name: string }; | ||||||
apiVersion: string; | ||||||
kind: string; | ||||||
}>(yamlStr); | ||||||
strictEqual(obj.data.mode, 420); | ||||||
}); | ||||||
|
||||||
it('should treat octal as string if version 1.2 is provided', () => { | ||||||
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. I'm curious why this test is so much different from the one before it. Couldn't it be the exact same setup, but with a different YAML version specified and a different expected value in the assertion? |
||||||
const yamlStr = ` | ||||||
apiVersion: v1 | ||||||
kind: ConfigMap | ||||||
metadata: | ||||||
name: test | ||||||
data: | ||||||
mode: '0644' | ||||||
`; | ||||||
|
||||||
const objs = loadAllYaml(yamlStr, { version: '1.2', maxAliasCount: 100, prettyErrors: true }); | ||||||
|
||||||
strictEqual(objs[0].data.mode, '0644'); | ||||||
}); | ||||||
|
||||||
it('should load multiple documents with default YAML 1.1', () => { | ||||||
const yamlStr = ` | ||||||
apiVersion: v1 | ||||||
kind: ConfigMap | ||||||
metadata: | ||||||
name: cm1 | ||||||
--- | ||||||
apiVersion: v1 | ||||||
kind: ConfigMap | ||||||
metadata: | ||||||
name: cm2 | ||||||
`; | ||||||
const objs = loadAllYaml(yamlStr) as Array<{ metadata: { name: string } }>; | ||||||
strictEqual(objs.length, 2); | ||||||
strictEqual(objs[0].metadata.name, 'cm1'); | ||||||
strictEqual(objs[1].metadata.name, 'cm2'); | ||||||
}); | ||||||
}); |
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 believe this is still a breaking change to end users. Prior to this change, they could pass in
LoadOptions
from thejs-yaml
library. After this change, the available options are different and there is no logic to support the old options.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.
@cjihrig ,I have reviewed the documentation for both libraries and compared their options. I found that most of the options have the same functionality, but a few methods differ in behavior. Do you think we could use the Adapter Design Pattern to address this issue and maintain compatibility while migrating to the new library?
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 think we need to decide what options we want to support and go from there. If we want to support the options from the previous library, then we will need an adapter. If we want to support the options from the new library, then we'll need to hold this PR until the next major release. I think no matter which route we go, we should define our own type for the options (as you've done here) so we can help insulate ourselves from this sort of problem in the future.
cc: @brendandburns @mstruebing
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.
Thanks, that’s clear. Let’s hold off until we get feedback from @brendandburns and @mstruebing about which option set we want to support going forward.
Uh oh!
There was an error while loading. Please reload this page.
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 definitely think we should define our own options as @cjihrig already said.
I would think we should make this PR fine and hold it until the next release except someone thinks it's worth to put in the effort to make this mergeable without a breaking change, but I would be fine with waiting.
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.
Thanks @cjihrig @mstruebing, I understand your point. It might take me some time, but I’d like to work on modifying this PR to avoid the breaking change. I’ll try to achieve that by adding an adapter layer to maintain backward compatibility.