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

kubectl code cleanup: overhaul the resource alias introduced #6573 #22337

Closed
caesarxuchao opened this issue Mar 2, 2016 · 10 comments
Closed

kubectl code cleanup: overhaul the resource alias introduced #6573 #22337

caesarxuchao opened this issue Mar 2, 2016 · 10 comments
Labels
area/kubectl kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@caesarxuchao
Copy link
Member

#6573 introduces the alias for resources. As our code base evolves, this code needs overhaul. I've noticed several problems:

  1. aliasToResource is a global variable, so every DefaultRESTMapper has the same alias map. https://github.com/kubernetes/kubernetes/blob/master/pkg/api/meta/restmapper.go#L492
    (This hasn't caused any bug yet because the problem is hidden by SplitResourceArgument)
  2. So far the only alias we have is "all", added in https://github.com/kubernetes/kubernetes/blob/master/pkg/api/install/install.go#L119, which will be translated to "rc", "svc", "pods", "pvc". I think this map needs to be updated. And I'd suggest use another alias, because kubectl also has a flag --all, which is confusing.

FWIW, I thought about using our ShortcutExpander to replace the alias code, but it doesn't seem to work because they are in different stages of the cmdline argument processing.

@Kargakis @janetkuo
@0xmichalis
Copy link
Contributor

cc: @deads2k @kubernetes/kubectl

@deads2k
Copy link
Contributor

deads2k commented Mar 2, 2016

I don't see aliases are related the RESTMapper. Aliases are about mapping one virtual resource to multiple actual resources (probably map[string][]GroupResource) and will only work effectively if the code making use of them understands the one to many expansion which is distinct from all the other RESTMapper functions.

I think I'd like to see a new interface for the expansion plumbed through where it's used as opposed to piggy-backing on the RESTMapper.

@caesarxuchao caesarxuchao added this to the next-candidate milestone Mar 3, 2016
@caesarxuchao
Copy link
Member Author

Thanks @deads2k. I agree it shouldn't be part of the RESTMapper.

This was referenced Mar 12, 2016
Merged
Merged
Closed
@bgrant0607 bgrant0607 added help-wanted priority/backlog Higher priority than priority/awaiting-more-evidence. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/kubectl labels Apr 27, 2016
@bgrant0607 bgrant0607 removed this from the next-candidate milestone Apr 27, 2016
@bgrant0607
Copy link
Member

Resource aliases are discussed in #18023 and #20293.

@bgrant0607 bgrant0607 added kind/bug Categorizes issue or PR as related to a bug. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels May 9, 2016
@janetkuo
Copy link
Member

"kubectl run" changed behavior to create deployments by default. A user mentioned in #23604 (comment):

... deployments do not list in 'kubectl get all', which only added to the confusion.

@0xmichalis
Copy link
Contributor

We should use the discovery client when we start combining resources from different api groups into the same aliases.

@dims dims mentioned this issue Jul 14, 2016
Merged
dims added a commit to dims/kubernetes that referenced this issue Jul 25, 2016
@dims
Extend all to more resources
ccf4e4d 
Added more things from the list here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cmd.go#L159

Update the devel/kubectl-conventions.md with the rules mentioned by
a few folks on which resources could be added to the special 'all' alias

Related to a suggestion in issue kubernetes#22337
k8s-github-robot pushed a commit that referenced this issue Aug 1, 2016
Merge pull request #28955 from dims/roadmap-extend-all
95b4572 
Automatic merge from submit-queue

Extend all to more resources

Added more things from the list here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cmd.go#L159

Note, did not add events as it did not seem useful to me. Since
this is just a list of messages generated by the system.

Related to a suggestion in issue #22337
@dims dims mentioned this issue Aug 7, 2016
Merged
xingzhou pushed a commit to xingzhou/kubernetes that referenced this issue Dec 15, 2016
@dims
Extend all to more resources
4783107 
Added more things from the list here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cmd.go#L159

Update the devel/kubectl-conventions.md with the rules mentioned by
a few folks on which resources could be added to the special 'all' alias

Related to a suggestion in issue kubernetes#22337
xingzhou pushed a commit to xingzhou/kubernetes that referenced this issue Dec 15, 2016
Merge pull request kubernetes#28955 from dims/roadmap-extend-all
a230a6f 
Automatic merge from submit-queue

Extend all to more resources

Added more things from the list here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cmd.go#L159

Note, did not add events as it did not seem useful to me. Since
this is just a list of messages generated by the system.

Related to a suggestion in issue kubernetes#22337
@calebamiles calebamiles modified the milestone: v1.6 Mar 8, 2017
@ethernetdan ethernetdan modified the milestone: v1.6 Mar 10, 2017
@k8s-github-robot k8s-github-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label May 31, 2017
@0xmichalis
Copy link
Contributor

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jun 16, 2017
@0xmichalis 0xmichalis removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. team/ux (deprecated - do not use) labels Jun 16, 2017
@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.

Prevent issues from auto-closing with an /lifecycle frozen comment.

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 Dec 28, 2017
@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 Jan 27, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/kubectl kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@calebamiles @caesarxuchao @0xmichalis @ethernetdan @janetkuo @bgrant0607 @deads2k @k8s-github-robot @k8s-ci-robot @fejta-bot