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.

Sign up for GitHub

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

Jump to bottom

kubernetes: bundle, not vendor, cpuset code #207

Merged
merged 3 commits into from
Aug 21, 2023
Merged

kubernetes: bundle, not vendor, cpuset code #207

merged 3 commits into from
Aug 21, 2023

Conversation

ffromani
Copy link
Contributor

in order to remove all dependencies on k8s.io/kubernetes, bundle (= copy/paste in the tree) the cpuset code instead of vendoring. We can just revert back to vendoring when we can update to k8s.io/utils compatible to 1.28, so worst case when we bump to kube 1.28.

@ffromani
k8s: import code, removing from vendor
5ccdf45 
Import `cpuset` package and copy in the tree up until
we can vendor `k8s.io/utils` compatible with version 1.28.

This is another step to get rid of the `k8s.io/kubernetes` dependency.

Signed-off-by: Francesco Romani <[email protected]>
@ffromani
podres: middleware: switch to imported cpuset
6924705 
trivial move + fix to the imported, and more up to date,
cpuset code. This will allow us to be more forward compatible
when we will switch again to the vendored `k8s.io/utils` code.

Signed-off-by: Francesco Romani <[email protected]>
@ffromani
vendor: update
61704aa 
remove the now-unneeded `cpuset` vendored dep.

Signed-off-by: Francesco Romani <[email protected]>
@ffromani ffromani requested review from swatisehgal and Tal-or August 10, 2023 07:27
@ffromani
Copy link
Contributor Author

final step and the engine that is propelling all this work: #206 (depends on this one, WIP)

@Tal-or
Copy link
Contributor

Tal-or commented Aug 13, 2023

What is the justification of getting rid of the k8s deps?

@ffromani
Copy link
Contributor Author

What is the justification of getting rid of the k8s deps?

The main driver is to be able to get rid of import k8s.io/kubernetes, which we did in the past and which we do in other project. Importing kube packages as libraries this way is not supported (actually discouraged) from k8s project and can break anytime. Our saving grace was (is) we just need a very narrow set of packages which happen to be quite stable.

@Tal-or
Copy link
Contributor

Tal-or commented Aug 21, 2023 •
edited
Loading

What is the justification of getting rid of the k8s deps?

The main driver is to be able to get rid of import k8s.io/kubernetes, which we did in the past and which we do in other project. Importing kube packages as libraries this way is not supported (actually discouraged) from k8s project and can break anytime. Our saving grace was (is) we just need a very narrow set of packages which happen to be quite stable.

So why not just wait for 1.28 and it will remove automatically because cpuset goes under k8s/utils?
we'll still need to change the imports path, but we'll do that anyway according to the PR description.

@ffromani
Copy link
Contributor Author

What is the justification of getting rid of the k8s deps?

The main driver is to be able to get rid of import k8s.io/kubernetes, which we did in the past and which we do in other project. Importing kube packages as libraries this way is not supported (actually discouraged) from k8s project and can break anytime. Our saving grace was (is) we just need a very narrow set of packages which happen to be quite stable.

So why not just wait for 1.28 and it will remove automatically because cpuset goes under k8s/utils? we'll still need to change the imports path, but we'll do that anyway according to the PR description.

that's also an option. It's up to us (maintaining the project) to decide if we want to have this bundling (expected to be stable and low-effort) for a while or just wait for 1.28.
Just note this PR is a prerequisite for #206

@Tal-or
Copy link
Contributor

Tal-or commented Aug 21, 2023

/lgtm
/approve
Then let's move on and merge it.
When 1.28 time comes we'll deal it

@ffromani ffromani merged commit 7e07961 into main Aug 21, 2023
@ffromani ffromani deleted the bundle-cpuset branch August 21, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swatisehgal swatisehgal Awaiting requested review from swatisehgal

@Tal-or Tal-or Awaiting requested review from Tal-or

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ffromani @Tal-or