use gopkg.in/laverya/yaml.v3 instead of gopkg.in/yaml.v2

[laverya]

this supports 64 bit floats and longer string lines
see #18

(laverya/yaml.v3 instead of yaml.v3 because the fork supports setting line length, and the original has no releases to pin)

A disadvantage of this upgrade is that there is no longer UnmarshalStrict - all Unmarshal calls are now strict. As mentioned later in the thread, this likely constitutes a breaking change.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: laverya
To complete the pull request process, please assign dims
You can assign the PR to them by writing /assign @dims in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[laverya]

I missed the addition of JSONObjectToYAMLObject. That's going to be interesting to port to yaml.v3, since the type it uses (yaml.MapSlice) no longer exists in v3...

[laverya]

And so here is a version that keeps go-yaml/yaml.v2 around for use with JSONObjectToYAMLObject. I'm not entirely happy with this, but I'm not sure if there's any other way to move to go-yaml/yaml.v3

[dims]

cc @deads2k @liggitt

[liggitt]

cc @sttts

[sttts]

Background: we need JSONObjectToYAMLObject and the (exact) v2 MapSlice type to interface with https://github.com/googleapis/gnostic.

[sttts]

A disadvantage of this upgrade is that there is no longer UnmarshalStrict - all Unmarshal calls are now strict.

What does this mean in practice? Which existing UX does this break?

[laverya]

@sttts

A disadvantage of this upgrade is that there is no longer UnmarshalStrict - all Unmarshal calls are now strict.

What does this mean in practice? Which existing UX does this break?

It means that the yaml parser will throw an error if duplicate keys are detected. I do not know of any specific use cases that this would break, though - besides the obvious one of "parse whatever junk the users threw at me". On reflection, this may constitute a breaking API change.

There is still a difference between kubernetes-sigs/yaml's Unmarshal and UnmarshalStrict functions, though - namely, UnmarshalStrict will throw an error if fields exist in the object being converted that do not exist in the destination object.

Background: we need JSONObjectToYAMLObject and the (exact) v2 MapSlice type to interface with https://github.com/googleapis/gnostic.

Ouch. That project seems to use MapSlice a lot, and it's never fun to rely on a type/method/etc that gets removed by a major version bump...

[neolit123]

all Unmarshal calls are now strict.

was strict unmarshalling actually enabled by default without a toggle?
this can be "quite" breaking for the k8s space if we move to v3.

looks like, yes:
https://github.com/go-yaml/yaml/blob/v3/yaml.go#L88
https://github.com/go-yaml/yaml/blob/v2/yaml.go#L80-L88

EDIT: no it does not, it still passes strict=false to unmarshal

[neolit123]

why include v3 from your fork. what differences it has from upstream?

[laverya]

Two major ones - the first is that it has tags
The second is the inclusion of go-yaml/yaml#455 - "Add a function to set the desired line length of an Encoder" which would resolve kubernetes-sigs/kustomize#947

[laverya]

The full diff is here: go-yaml/yaml@v3...laverya:v3.0.0-beta1
7 additional commits

[laverya]

all Unmarshal calls are now strict.

was strict unmarshalling actually enabled by default without a toggle?
this can be "quite" breaking for the k8s space if we move to v3.

Sounds like adding an explicit "UnmarshalUnsafe" function might be worthwhile in go-yaml/yaml.v3, then.

[neolit123]

Sounds like adding an explicit "UnmarshalUnsafe" function might be worthwhile in go-yaml/yaml.v3, then.

negating API methods is breaking too. at least the change was part of a MAJOR, yet this leaves us in a space where we have to fork v3 if we want to move to it.

[laverya]

negating API methods is breaking too. at least the change was part of a MAJOR, yet this leaves us in a space where we have to fork v3 if we want to move to it.

I meant something more along the lines of "add the negation method to the upstream, and use it to keep kubernetes-sigs/yaml's methods the same". And since this is already based on a fork, what's one more change, right? 😆 (In all seriousness, this feels like something that should get added back into the upstream)

[laverya]

@neolit123 unmarshal behavior with duplicate keys should be restored now through the use of yaml3.UnmarshalPermissive

[neolit123]

thanks for the update @laverya ,
one of the reasons we moved this yaml package under sigs.k8s.io as a fork is to not use third party/private dependency in a very core area of the project.

with this PR we are again introducing a private repository as a dependency.
but at this point will defer to the approvers whether we should proceed with the PR.

[laverya]

one of the reasons we moved this yaml package under sigs.k8s.io as a fork is to not use third party/private dependency in a very core area of the project.

with this PR we are again introducing a private repository as a dependency.

That makes perfect sense, and honestly I would not want my fork as a key dependency either 😆

I'd initially intended to wait for the upstream to merge things before making this PR, but as time goes on that's looking more and more like it won't happen without some sort of external impetus - and this could be that impetus. Postponing a merge until we can switch back to go-yaml/yaml would make sense to me, but I think the rest of the code could still be reviewed today. (And this also provided a good look at where the current go-yaml/yaml.v3 implementation falls short, like the lack of a permissive unmarshal function)

[fejta]

How would we feel about creating a v3 branch of this repo that uses (the patched) yaml.v3?

This will downstream repos to safely start opting into the new behavior by changing their import path

[laverya]

I'd be happy with that! Either with a 'laverya' dependency or something actually owned by kubernetes

[thaJeztah]

I opened #29 to update the v2 version to v2.2.7

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[k8s-ci-robot]

@laverya: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[inteon]

gnostic now also uses yaml.v3: google/gnostic#194

[horacimacias]

is there any chance in reopening this or creating a new PR to have this done?
I'm being hit by yaml 1.1 vs 1.2 differences and it sounds like this migration to yaml.v3 should help if not completely fix the issues.

[liggitt]

is there any chance in reopening this or creating a new PR to have this done? I'm being hit by yaml 1.1 vs 1.2 differences and it sounds like this migration to yaml.v3 should help if not completely fix the issues.

#76 is plan to update this repo to use yaml.v3 while preserving compatibility with existing formatting

That PR is pending on some remaining comments - see #76 (comment)