Create a fork of go-yaml

[natasha41575]

This PR creates forks of go-yaml v2 and go-yaml v3.

Related issue: #72

This forks and also contains the changes in go-yaml/yaml#753 that are necessary for kustomize (and are the reason we need to make the fork).

I've added @KnVerey and myself as owners; as discussed in meeting we are happy to maintain this. I also added the disclaimer stating that we will not accept large changes here.

[natasha41575]

/cc @KnVerey
/cc @liggitt
/cc @lavalamp

[liggitt]

It might be worth setting up apidiff presubmits on the go yaml subpackage trees to ensure we don't break go API compatibility on changes accidentally, since I expect these sub packages to attract a lot of usage. xref https://github.com/kubernetes/utils/blob/master/.github/workflows/test.yml#L52

[natasha41575]

@liggitt I have addressed most of your comments, but still have a couple of questions:

1. How to add the licenses from go-yaml to the root license? Should I just append the contents, or is there a particular delimiter or heading I should use?

2. To set up the api-diff, would there be any issues with copying the script https://github.com/kubernetes/utils/blob/master/hack/verify-apidiff.sh into this repo?

[liggitt]

• How to add the licenses from go-yaml to the root license? Should I just append the contents, or is there a particular delimiter or heading I should use?

I'm... not sure... the file at goyaml.v3/LICENSE already enumerates multiple licenses and specific files. Maybe that's an example to follow, but I really don't have expertise in the implications/requirements here. @dims / @cblecker - any insight here? We want to make sure we're complying with licenses of code being included, and also minimize issues with license scanners, etc, from folks consuming the project.

2. To set up the api-diff, would there be any issues with copying the script https://github.com/kubernetes/utils/blob/master/hack/verify-apidiff.sh into this repo?

Yeah, copy whatever is useful. Will probably need some tweaks to make it pay attention just to the two go yaml subpackages

[dims]

I'm... not sure... the file at goyaml.v3/LICENSE already enumerates multiple licenses and specific files. Maybe that's an example to follow, but I really don't have expertise in the implications/requirements here. @dims / @cblecker - any insight here? We want to make sure we're complying with licenses of code being included, and also minimize issues with license scanners, etc, from folks consuming the project.

Let's start with what is already in goyaml.v3/LICENSE, if it trips up tools we use, we can then go figure out whether to fix the tool or fix the file.

[cblecker]

We should consider using the third_party structure, similar to k/k: https://github.com/kubernetes/kubernetes/tree/master/third_party

Tooling is set up to handle that

[liggitt]

that's where @natasha41575 started, but these are permanent forks that will be referenced by other downstreams, it's weird to expose a third_party package path to them - #76 (comment)

[cblecker]

it's weird to expose a third_party package path to them

I think that's the whole point of third_party/forked is to call out that this is a fork of something else we didn't write. That's part of the Apache 2.0 licence requirements to my understanding (standard I am not a lawyer disclaimer).

[liggitt]

That's part of the Apache 2.0 licence requirements to my understanding

Really? I've never see a directory structure requirement like that. I think pushing a "sigs.k8s.io/third_party/yaml.v2" import path to downstreams forever is really ugly, and sort of confusing, and would like to avoid it if at all possible. Not sure who could give a definitive answer on what's allowed here

[dims]

@cblecker i haven't had that called out in AL 2.0, it's for our convenience we have the extra third_party so we can be careful when we touch code in there.

I agree with @liggitt that this is too ugly to impose on downstream folks.

@liggitt i've opened cncf/foundation#354 until there is direction otherwise, we can keep doing what is best for folks that use us.

[cblecker]

@dims sorry, I guess I skipped a mental step in my thought process

So Apache 2.0 requires, in section 4(b), that any modifications have prominent notices that they've been modified. So third_party, like vendor, calls out where code is copied into this repo so it can be treated differently (with additional notices on modification).

Again though, this is my "IANAL" understanding :)

[dims]

@cblecker no worries, i got it on record from ChrisA in cncf/foundation#354 we are good to go here.

[liggitt]

also seems worth noting somewhere what version of yaml.v3 this is synced to, and pulling in https://github.com/go-yaml/yaml/releases/tag/v3.0.1 in this first sync

[liggitt]

I still can't figure out how to exercise this path in a unit test

if we can't exercise this path, let's drop it for now, rather than possibly modify indentation in a way that will break something. We can always add it (with a test) if we figure out whether it's needed in this path as well.

Adjust dropping this in the make sequence style configurable commit

[natasha41575]

Done.

[liggitt]

testing this in k/k in kubernetes/kubernetes#121406 (the dependencies linter is expected to fail while it's pointing at a temp branch, but the other tests should pass)

[liggitt]

looks like the unit test failures in kubernetes/kubernetes#121406 are due to messages changes already present at HEAD (before this PR) added in https://github.com/kubernetes-sigs/yaml/pull/65/files#diff-acfe93d173e77894feaede834d0927584abe0fc551dba248d191e16edaa63611

some of those are just wrong (like the doubled error converting YAML to JSON: error converting YAML to JSON: yaml: line 13: could not find expected ':' bit)

others are just changing messages without necessarily improving things:

expected err: 
        strict decoding error: yaml: unmarshal errors:
          line 7: key "num" already set in map
got err: 
        strict decoding error: error converting YAML to JSON: yaml: unmarshal errors:
          line 7: key "num" already set in map

I'd like to back out some of the extra verbosity, but I can't tell if kustomize has taken a hard dependency on those strings existing in errors (https://github.com/kubernetes-sigs/kustomize/blob/0f2618b21d3f08c2b2c4ecced292469747ad3354/api/internal/kusterr/yamlformaterror.go#L49-L51)

Opened #96 to poke at this

[natasha41575]

Rebased to pick up #96

[liggitt]

/lgtm
/approve

all relevant tests in kubernetes/kubernetes#121406 passed on this

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, natasha41575

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [liggitt]

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

[rfay]

Now we have a fork, which is great. But all of us using go-yaml v3 want to know how to follow this and what the changing instructions will be.

[natasha41575]

@rfay I think we will need to wait for a release before we can use it properly (unless you pin to this commit).

Then you will have to replace all instances of import "gopkg.in/yaml.v3" to import "sigs.k8s.io/yaml/goyaml.v3" with a release that includes this fork and it should work.

kubernetes-sigs/kustomize@11b76be shows what this will end up looking like in kustomize (though that one is pinned to my branch, which will go away when the release is available).

[reinvantveer]

@natasha41575 @liggitt and all involved: congrats and thank you for your efforts. I believe this is a vital step in the k8s ecosystem allowing it to move towards standardisation of the primary means to deploy k8s resources: through yaml manifests. Yaml that conforms to a yaml standard, instead of a goyaml "dialect". The path on how to maintain backwards compatibility isn't quite clear yet, but this is a good step.