fix(yaml): add support for YAML 1.1 parsing in Kubernetes manifests #2660
Conversation
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Tanmayshi 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
cncf-cla: no
| deepEqual(actual, expected); | ||
| }); | ||
|
|
||
| it('should parse octal values correctly using default YAML 1.1', () => { |
There was a problem hiding this comment.
| it('should parse octal values correctly using default YAML 1.1', () => { | |
| it('should parse octal values as numbers using default YAML 1.1', () => { |
| strictEqual(obj.data.mode, 420); | ||
| }); | ||
|
|
||
| it('should treat octal as string if version 1.2 is provided', () => { |
There was a problem hiding this comment.
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?
| */ | ||
| export function loadYaml<T>(data: string, opts?: yaml.LoadOptions): T { | ||
| const yml = yaml.load(data, opts) as any as KubernetesObject; | ||
| export function loadYaml<T>(data: string, opts?: YamlParseOptions): T { |
There was a problem hiding this comment.
I believe this is still a breaking change to end users. Prior to this change, they could pass in LoadOptions from the js-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.
@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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
|
It looks like the CLA still needs to be signed. |
4 participants
##Summary
This PR fixes YAML parsing inconsistencies by replacing js-yaml with the modern yaml library, which supports both YAML 1.1 and 1.2 specifications.
##What’s Changed
Replaced js-yaml with yaml parser.
Added support for parsing multiple YAML documents using both YAML 1.1 and 1.2 versions.
Updated loadYaml and loadAllYaml functions to accept YamlParseOptions with version selection.
Fixes #2539