Skip to content
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

semantic: add forked reflect based DeepEqual #80

Merged
merged 1 commit into from Feb 7, 2019

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Feb 6, 2019

We used to have this in apimachinery and in kube itself. The actual code is kube-independent.

We need this in kube-openapi. Before introducing another fork, we should move it here, for easy consumption for any party.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 6, 2019
@sttts
Copy link
Contributor Author

sttts commented Feb 6, 2019

/assign @dims

@sttts sttts force-pushed the sttts-semantic-deepequal branch 2 times, most recently from 7b6e69c to 1868c7e Compare February 6, 2019 12:10
@dims
Copy link
Member

dims commented Feb 6, 2019

  • could we somehow denote what code changed?
  • add separate tests for the updated/new behavior?

@dims
Copy link
Member

dims commented Feb 6, 2019

never mind this code was picked from k/k and staging ... let's add the third_party/forked in the directory structure please

[dims@dims-mac 07:31] ~/go/src/k8s.io/kubernetes ⟩ find . -name deep_equal*.go | xargs ls -altr
-rw-r--r--  1 dims  staff  1058 Nov 16 16:47 ./staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go
-rw-r--r--  1 dims  staff  9789 Nov 16 16:47 ./staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go
-rw-r--r--  1 dims  staff  3659 Nov 16 16:47 ./staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go
-rw-r--r--  1 dims  staff  9789 Nov 16 16:47 ./third_party/forked/golang/reflect/deep_equal.go
-rw-r--r--  1 dims  staff  3659 Nov 16 16:47 ./third_party/forked/golang/reflect/deep_equal_test.go

@dims
Copy link
Member

dims commented Feb 6, 2019

/assign @thockin

i agree that the code has diverged a lot from what it was in the go repo. Tim, WDYT about the directory names (do we need third_party/forked)?

@thockin
Copy link
Member

thockin commented Feb 6, 2019

The third_party designation was to satisfy legal. I would want a legal-ish opinion on this. @dankohn is there an alias we can tag for consultation?

@dankohn
Copy link

dankohn commented Feb 6, 2019

Yes, @swinslow is happy to help. My read is that it BSD-licensed code and so cannot be part of the main project, but it's fine to keep it in a third-party folder, as BSD is one of the whitelisted licenses (and the code has other users). But Steve can give the definitive thumbs up.

@swinslow
Copy link

swinslow commented Feb 6, 2019

I'd recommend keeping it in a folder that is called "third_party/forked/" or similar, along the lines of what @dankohn described. Longer comments follow below:

The ideal long-term solution would be to upstream the changes where possible, particularly if they'd be usable for others upstream. That way the version used would be "unmodified" from Kubernetes' perspective, and we wouldn't be maintaining a separate fork. But I know there's no guarantee they'd be pulled in, or that we'd want to wait for that to happen.

The CNCF whitelist process (which we'll be posting more visibly in sig-release/licensing) automatically approves dependencies that are under certain non-Apache permissive licenses and meet some other criteria. One of those criteria is that the dependency be located in a third-party folder (e.g., dependencies in /vendor/) so it isn't intermingled with Apache-2.0 project code. Another criteria for the whitelist, though, is that it only applies where the dependency is unmodified. Since the dependency is being modified here, the forked version isn't automatically whitelisted.

However, having the forked dependency in a separate folder anyway does help put people on notice that it might not be Apache-2.0 code. That's part of why the CNCF IP Committee has recommended approving license exceptions for these BSD-3-Clause forked components currently found in the "/third_party/forked/" directory. So I would recommend continuing this practice.

@sttts
Copy link
Contributor Author

sttts commented Feb 7, 2019

I moved the code to third_party/forked/golang/reflect. Ptal.

@dims
Copy link
Member

dims commented Feb 7, 2019

/approve
/lgtm

Thanks @dankohn @swinslow @thockin

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit 52ad730 into kubernetes:master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants