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 label Feb 6, 2019
@k8s-ci-robot k8s-ci-robot requested review from apelisse and thockin Feb 6, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL label Feb 6, 2019
@sttts
Copy link
Contributor Author

@sttts 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 Feb 6, 2019
@dims
Copy link
Member

@dims 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 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 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 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 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 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 sttts force-pushed the sttts-semantic-deepequal branch from 1868c7e to 47bdc18 Compare Feb 7, 2019
@sttts
Copy link
Contributor Author

@sttts sttts commented Feb 7, 2019

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

@sttts sttts force-pushed the sttts-semantic-deepequal branch from 47bdc18 to 0a7e25b Compare Feb 7, 2019
@sttts sttts force-pushed the sttts-semantic-deepequal branch from 0a7e25b to 3e0f8d3 Compare Feb 7, 2019
@dims
Copy link
Member

@dims dims commented Feb 7, 2019

/approve
/lgtm

Thanks @dankohn @swinslow @thockin

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 7, 2019
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Feb 7, 2019

[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 label Feb 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit 52ad730 into kubernetes:master Feb 7, 2019
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cncf-cla: yes lgtm size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants