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

Find home for utils used by kubectl #48209

Closed
pwittrock opened this issue Jun 28, 2017 · 46 comments
Closed

Find home for utils used by kubectl #48209

pwittrock opened this issue Jun 28, 2017 · 46 comments
Assignees
@lavalamp
Labels
area/code-organization Issues or PRs related to kubernetes code organization kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@pwittrock
Copy link
Member

Kubectl depends on several util libraries, and they need to be moved out of the k/kubernetes repo for kubectl to move out without vendoring k/kubernetes. We are blocked on making progress on this until we have a home for the utils.

@bgrant0607 @lavalamp @smarterclayton @thockin @deads2k @liggitt Would love to get your perspective.

Move to api-machinery first. Eventually move all utils in api-machinery to another repo.

  • A number of common utilities have already moved to api-machinery. We should be consistent with where they are moved to.
  • Everything can depend on api-machinery.
  • Doesn't block on setting up a new repo and process for vendoring into kubernetes/kubernetes

Move to another util repo

  • Simple proof of concept for moving something out of the main repo without tackling some of the harder problems (e2e testing, needing to transactionally update multiple repos)
  • Provides a stdlib for other go projects to use without using Kubernetes libraries
  • Won't need to update go packages 2x (once for each move)

List of util packages:

  • k8s.io/kubernetes/pkg/util
  • k8s.io/kubernetes/pkg/util/crlf
  • k8s.io/kubernetes/pkg/util/exec
  • k8s.io/kubernetes/pkg/util/i18n
    • k8s.io/kubernetes/pkg/generated
  • k8s.io/kubernetes/pkg/util/interrupt
  • k8s.io/kubernetes/pkg/util/logs
  • k8s.io/kubernetes/pkg/util/slice
  • k8s.io/kubernetes/pkg/util/term
@pwittrock pwittrock added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Jun 28, 2017
@pwittrock pwittrock added this to the v1.8 milestone Jun 28, 2017
@pwittrock
Copy link
Member Author

cc @apelisse @monopole

@bgrant0607 bgrant0607 added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Jun 28, 2017
@smarterclayton
Copy link
Contributor

smarterclayton commented Jun 28, 2017 via email

@lavalamp
Copy link
Member

I am not in favor of apimachinery becoming a dumping ground for these, even temporarily.

We already got consensus on this a while back in an email thread on kubernetes-dev, I will try to dig it up.

@deads2k
Copy link
Contributor

deads2k commented Jun 28, 2017

Some of these are only used by kubectl or logical dependents thereof. I'd like to see those contained to kubectl until and if they are needed by another repo. For instance:

  1. k8s.io/kubernetes/pkg/util/i18n
  2. k8s.io/kubernetes/pkg/generated - particularly egregious since it actually include kubectl specific command information
  3. k8s.io/kubernetes/pkg/util/logs - the only other dependent is a custom controller which google promised to move out of tree some time ago
  4. k8s.io/kubernetes/pkg/util/crlf

I don't want single use utils dropped into a repo like apimachinery.

@lavalamp
Copy link
Member

Please look at this email thread: https://groups.google.com/d/msg/kubernetes-dev/jvmDxc29BOw/UElLJhQtBgAJ

@pwittrock
Copy link
Member Author

@mengqiy

cc @kubernetes/sig-cli-maintainers

@pwittrock
Copy link
Member Author

@deads2k I am good with moving utils only consumed by kubectl under pkg/kubectl/util. What about the others? What is the plan for things used by printers? IIUC, the printer libraries were moved out of kubectl for server-side printing, so things used by the printer libraries can't live in kubectl.

@deads2k
Copy link
Contributor

deads2k commented Jun 29, 2017

@deads2k I am good with moving utils only consumed by kubectl under pkg/kubectl/util. What about the others? What is the plan for things used by printers? IIUC, the printer libraries were moved out of kubectl for server-side printing, so things used by the printer libraries can't live in kubectl.

I think those are more artifacts of our how our API server handles requests than code that is related to the type structure, serialization, and conversion. I don't think they belong in apimachinery either.

The mostly likely bit of code you'll want to share (the printer and describe impls themselves for compatibility with older servers) definitely doesn't belong in apimachinery since it is type specific.

This was referenced Jun 29, 2017
Merged
Merged
Merged
@mengqiy
Copy link
Member

mengqiy commented Jun 29, 2017 •
edited

Filed a few PRs:
k8s.io/kubernetes/pkg/util: #48295
k8s.io/kubernetes/pkg/util/crlf: #48298
k8s.io/kubernetes/pkg/util/term: #48299
k8s.io/kubernetes/pkg/util/slice: #48304

@mengqiy
Copy link
Member

mengqiy commented Jun 29, 2017

@brendanburns Can we move k8s.io/kubernetes/pkg/util/i18n to kubectl?

k8s.io/kubernetes/pkg/util/logs - the only other dependent is a custom controller which google promised to move out of tree some time ago

@deads2k kubefed and test/e2e/e2e.go also depend on this. Can we move it to client-go?

@smarterclayton
Copy link
Contributor

smarterclayton commented Jun 30, 2017 via email

@deads2k
Copy link
Contributor

deads2k commented Jun 30, 2017

@deads2k kubefed and test/e2e/e2e.go also depend on this. Can we move it to client-go?

Those two can depend on kubectl

@mengqiy mengqiy mentioned this issue Jun 30, 2017
Merged
@thockin
Copy link
Member

thockin commented Jun 30, 2017

I think the ideal state is that k8s.io/util/* holds a set of libs that are really plausibly generic, well tested, well factored, and meet some minimum complexity bar (e.g. Int32Ptr maybe doesn't. StringSlice probably doesn't).

I'm less concerned about whether it has a single user, though obviously more is better.

@apelisse
Copy link
Member

I think the ideal state is that k8s.io/util/* holds a set of libs that are really plausibly generic, well tested, well factored, and meet some minimum complexity bar

and maintain backward compatibility.

@thockin
Copy link
Member

thockin commented Jun 30, 2017 via email

k8s-github-robot pushed a commit that referenced this issue Jun 30, 2017
Merge pull request #48295 from mengqiy/kubectl_util
3dcd308 
Automatic merge from submit-queue (batch tested with PRs 48295, 48298, 47339, 44910, 48037)

eliminate kubectl dependency on k8s.io/kubernetes/pkg/util

Ref: #48209

/assign @apelisse @monopole 

cc: @pwittrock 
```release-note
NONE
```
k8s-github-robot pushed a commit that referenced this issue Jun 30, 2017
Merge pull request #48298 from mengqiy/kubectl_crlf
903a454 
Automatic merge from submit-queue (batch tested with PRs 48295, 48298, 47339, 44910, 48037)

move crlf to kubectl/util

move crlf from pkg/util/crlf to pkg/kubectl/util/crlf

Ref: #48209

```release-note
NONE
```
/assign @apelisse @monopole 

cc: @pwittrock
k8s-github-robot pushed a commit that referenced this issue Jul 1, 2017
Merge pull request #48299 from mengqiy/kubectl_term
73a94ea 
Automatic merge from submit-queue (batch tested with PRs 47918, 47964, 48151, 47881, 48299)

move term to kubectl/util

move term from pkg/util/term to pkg/kubectl/util/term

remove dependency of `k8s.io/kubernetes/pkg/util/term` for `pkg/kubelet/dockershim/exec.go` and `pkg/kubelet/dockershim/exec.go`

Ref: #48209

```release-note
NONE
```
/assign @apelisse @monopole 

cc: @pwittrock
k8s-github-robot pushed a commit that referenced this issue Jul 2, 2017
Merge pull request #48304 from mengqiy/kubectl_slice
950a09d 
Automatic merge from submit-queue

split util/slice for kubectl

Split util/slice to remove the dependency of kubectl on `k8s.io/kubernetes/pkg/util/slice`.

```release-note
NONE
```
/assign @apelisse @monopole 

cc: @pwittrock 

ref #48209
@bgrant0607
Copy link
Member

cc @monopole

@bgrant0607
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2018
dims pushed a commit to dims/kubernetes that referenced this issue Feb 8, 2018
Merge pull request kubernetes#49234 from mengqiy/master
25bba44 
Automatic merge from submit-queue (batch tested with PRs 49107, 47177, 49234, 49224, 49227)

Move util/exec to vendor

Move util/exec to vendor.
Update import paths.
Update godep

Part of kubernetes#48209

Associate PR against `k8s.io/utils` repo: kubernetes/utils#5

```release-note
NONE
```

/assign @apelisse
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 23, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 23, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@timfallmk
Copy link

Can we remove this permanently from lifecycling?

@apelisse
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Jun 28, 2018
@apelisse apelisse mentioned this issue Jul 25, 2018
Merged
@WanLinghao
Copy link
Contributor

k8s.io/kubernetes/pkg/util/term should be removed since no package refers it any more.
ref:#69515

@WanLinghao
Copy link
Contributor

@mengqiy hello, do we have any new progress about this? I am especially concerned about k8s.io/kubernetes/pkg/printers. Where should it move to?

@mengqiy
Copy link
Member

mengqiy commented Oct 10, 2018

/reopen

@k8s-ci-robot
Copy link
Contributor

@mengqiy: Reopening this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot reopened this Oct 10, 2018
@mengqiy
Copy link
Member

mengqiy commented Oct 10, 2018

do we have any new progress about this? I am especially concerned about k8s.io/kubernetes/pkg/printers. Where should it move to?

@juanvallejo knows the answer best :)
cc @seans3

@juanvallejo
Copy link
Contributor

juanvallejo commented Oct 10, 2018 •
edited

There is currently a kubernetes/utils repo for any non-kube-specific utils.

As for the printers package, it does not need to move anywhere. That package is used by the server to generate a Table response for server-side printing. Since there are some code-paths in kubectl that still rely on this package as well, we have discussed a few options:

  • Try to completely remove any remaining dependencies that kubectl has on the pkg/printers/internalversion package (I believe some fallback functionality in kubectl get might be the only culprit left here)
  • Duplicate all of the helpers under pkg/printers/internalversion into the kubectl tree, but modify all of the new copies to only deal with external versions

Any printer-flag related files outside of the internalversion package should be superseded by helpers in the new cli-runtime repo. Any kubectl commands that still rely on them should be updated to use the cli-runtime repo instead

@thockin thockin removed their assignment Feb 15, 2019
@bgrant0607
Copy link
Member

@seans3 Has this been resolved by moving kubectl into staging?

@dims
Copy link
Member

dims commented May 14, 2019

/area code-organization

@k8s-ci-robot k8s-ci-robot added the area/code-organization Issues or PRs related to kubernetes code organization label May 14, 2019
@dims
Copy link
Member

dims commented Jun 4, 2019

@pwittrock can you please reopen if needed?

/close

@k8s-ci-robot
Copy link
Contributor

@dims: Closing this issue.

In response to this:

@pwittrock can you please reopen if needed?

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lavalamp lavalamp

Labels
area/code-organization Issues or PRs related to kubernetes code organization kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests