-
Notifications
You must be signed in to change notification settings - Fork 75
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
Create a fork of go-yaml #76
Create a fork of go-yaml #76
Conversation
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 |
cb2240f
to
0e00b68
Compare
@liggitt I have addressed most of your comments, but still have a couple of questions:
|
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.
Yeah, copy whatever is useful. Will probably need some tweaks to make it pay attention just to the two go yaml subpackages |
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. |
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 |
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) |
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). |
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 |
@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. |
@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 :) |
@cblecker no worries, i got it on record from ChrisA in cncf/foundation#354 we are good to go here. |
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 |
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 others are just changing messages without necessarily improving things:
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 |
1ce3fd1
to
f2c0229
Compare
Rebased to pick up #96 |
/lgtm all relevant tests in kubernetes/kubernetes#121406 passed on this |
[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:
Approvers can indicate their approval by writing |
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. |
@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 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). |
@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. |
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.