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

test: remove k8s.io/kubernetes dependency by using containerd copy #984

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

thaJeztah
Copy link
Contributor

The k8s.io/kubernetes dependency is only needed for a single function
(GetAddressAndDialer), which doesn't seem to be in any module, other than
k8s.io/k8s itself (https://github.com/search?q=org%3Akubernetes+GetAddressAndDialer&type=code)

Containerd created a copy of this utility for that reason (as part of containerd/cri#1463), so let's use that copy to get rid of the dependency on k8s.io/k8s.

Perhaps we should try to have that utils package included in one of the smaller k8s.io moduless.

The k8s.io/kubernetes  dependency is only needed for a single function
(GetAddressAndDialer), which doesn't seem to be in any module, other than
k8s.io/k8s itself.

Containerd created a copy of this utility for that reason, so let's use that
copy to get rid of the dependency on k8s.io/k8s.

Perhaps we should try to have that utils package included in one of the
smaller k8s.io moduless.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Now that k8s.io/kubernetes is no longer used, this replace rule should
no longer be needed (`go mod tidy` and `go mod vendor` worked without
problem).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah requested a review from a team as a code owner March 25, 2021 09:50
@thaJeztah
Copy link
Contributor Author

@dims PTAL 😅

Perhaps we should try to have that utils package included in one of the smaller k8s.io module.

Perhaps you know a better place for this utility? Looking at it, the code only depends on k8s.io/cri-api (so may be a good place to put it), go-winio (for winio.ListenPipe() and k8s.io/klog (for a single warning; perhaps something that should be removed / changed)

@dims
Copy link
Contributor

dims commented Mar 25, 2021

@thaJeztah +1 to remove the single warning using k8s.io/klog in the containerd copy. the cri-api, still debating that as that repo is mostly for the API itself and this is a helper. for now let's leave this in containerd and use it in hcsshim

@kevpar
Copy link
Member

kevpar commented Mar 25, 2021

I'm curious if this is driven by general code hygiene, or if there is a specific reason to ensure we don't have a dependency on k8s.io/kubernetes? I know we don't want to depend on it in the main module, but I thought for the tests we didn't really care.

@thaJeztah
Copy link
Contributor Author

It is driven by code hygiene; it won't have a direct effect on production code, but I don't think any module (even test ones) should depend on k8s.io/kubernetes. I think this change also makes it a lot more transparent what's actually needed for this package, which can help future maintenance and prevent confusion.

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dcantah dcantah merged commit 64000d5 into microsoft:master Mar 25, 2021
@thaJeztah thaJeztah deleted the remove_k8s_k8s branch March 25, 2021 21:24
@TBBle
Copy link
Contributor

TBBle commented Mar 26, 2021

image
💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants