Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Delete direct dependency of kubernetes #48

Closed
liubog2008 opened this issue Oct 15, 2019 · 10 comments · Fixed by #74
Closed

Delete direct dependency of kubernetes #48

liubog2008 opened this issue Oct 15, 2019 · 10 comments · Fixed by #74

Comments

@liubog2008
Copy link

liubog2008 commented Oct 15, 2019

Now go.mod has dependency of kubernetes v1.11.2.
It will be hard to import job_controller/pkg/api because of compatibility with new version client-go

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.77. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@liubog2008
Copy link
Author

/bug

@liubog2008
Copy link
Author

$ go mod why k8s.io/client-go/util/integer 
go: finding k8s.io/api latest
go: finding k8s.io/apimachinery latest
# k8s.io/client-go/util/integer
xxx/cmd
github.com/kubeflow/mpi-operator/pkg/client/clientset/versioned
github.com/kubeflow/mpi-operator/pkg/client/clientset/versioned/typed/kubeflow/v1alpha2
github.com/kubeflow/mpi-operator/pkg/apis/kubeflow/v1alpha2
github.com/kubeflow/common/job_controller/api/v1
k8s.io/kubernetes/pkg/controller
k8s.io/client-go/util/integer

@gaocegege
Copy link
Member

We should to remove the dep here https://github.com/kubeflow/common/blob/master/job_controller/api/v1/controller.go#L83

Then we do not need "k8s.io/kubernetes/pkg/controller"

@liubog2008
Copy link
Author

There are two way to resolve this problem

  1. Fork controller.ControllerExpectationsInterface as third_party code
  2. Move JobController out of package api/v1 (why it is in v1 package ==)

@gaocegege
Copy link
Member

I have no idea why we place it in api.

@richardsliu @jian-he @merlintang @terrytangyuan Do you have idea about it?

@mmourafiq
Copy link

Any update on this issue, it's breaking dependency on all operators' api packages.

@Jeffwan
Copy link
Member

Jeffwan commented Mar 8, 2020

There are two way to resolve this problem

  1. Fork controller.ControllerExpectationsInterface as third_party code

em. this will work but probably not the elegant way, I notice there's an issue to suggest to add expectations in client-go. I reopen the issue and will spend some time on it later. kubernetes/client-go#332

  1. Move JobController out of package api/v1 (why it is in v1 package ==)

I doubt this will help resolve the dependency issue if you use go mod.? Correct me if I am wrong. Even you import github.com/kubeflow/common/job_controller/api/v1, dependencies in other packages will be fetched to dependency graph. But I agree it would be good to move type JobController out of api package. It should be part of the https://github.com/kubeflow/common/blob/master/job_controller/job_controller.go

I file a PR #51 to make the change.

More changes need to be done to upgrade client-go and Kubernetes dependency. I will file seperate PR to address the issue. There're some known incompatible issues prior to 1.15. modules did not include go.mod files, so go does not understand what versions of transitive dependencies to use. That makes it fetch the latest version of all transitive dependencies, which are not compatible. We should upgrade to >= 1.15 to resolve some issues/

@terrytangyuan
Copy link
Member

terrytangyuan commented Mar 9, 2020

I agree with moving it out of the package api/v1 (also approved #51). Others please double check as well.

@Jeffwan
Copy link
Member

Jeffwan commented Apr 15, 2020

Consider the case we plan to use PodControlInterface which is folked from https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/controller_utils.go#L464.

I think it make sense to folk expectation code as well, that's in controller_utils.go.

After that, we will remove all direct Kubernetes dependencies

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

Successfully merging a pull request may close this issue.

5 participants