Add completion in kubectl debug #130033
Add completion in kubectl debug #130033
Conversation
k8s-ci-robot
added
release-note
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
ardaguclu
force-pushed
the
completion-use-restclientgetter
branch
from
d675e6a to
533a121
Compare
k8s-ci-robot
added
size/XS
ardaguclu
changed the title
ardaguclu
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| @@ -383,6 +383,8 @@ func NewKubectlCommand(o KubectlOptions) *cobra.Command { | |||
| // Avoid import cycle by setting ValidArgsFunction here instead of in NewCmdGet() | |||
| getCmd := get.NewCmdGet("kubectl", f, o.IOStreams) | |||
| getCmd.ValidArgsFunction = utilcomp.ResourceTypeAndNameCompletionFunc(f) | |||
| debugCmd := debug.NewCmdDebug(f, o.IOStreams) | |||
There was a problem hiding this comment.
Are there import cycles for the debug command also? If not I suggest putting the ValidArgsFunction in the debug.go file where the debug command is being declared.
The get command is the only exception to that pattern because the completion code calls the get code back.
There was a problem hiding this comment.
debug command (just like with the other newer commands) expects restClientGetter not factory
. But completion util expects Factory (to pass through get command invocation).This is the only place where we actually have Factory.
There was a problem hiding this comment.
We can cast it in debug.go like;
f, ok := restClientGetter.(Factory); ok {
validargs = completion(f)
}which would be alternative.
There was a problem hiding this comment.
I haven’t paid attention to the evolution of the code recently, so whatever you feel is best.
There was a problem hiding this comment.
It's probably OK for this PR, but it seems like a problem that we can't define completions inside of a command that uses RESTClientGetter instead of Factory.
The root of the problem seems to be that get.Run() requires a Factory. I wonder how hard it would be to change get to use RESTClientGetter instread of Factory... 🤔
Then, all the functions in completions.go can be changed to use RESTClientGetter, making it more flexible to be used in other commands.
Like I said, I don't think it needs to be part of this PR, but it is probably something worth thinking about.
(BTW, I don't think we should try to cast restClientGetter to a Factory in debug.go... it might be fine for kubectl, but if someone else is using the debug command with a different implementation of the RESTClientGetter interface, they would be surprised by this).
There was a problem hiding this comment.
The root of the problem seems to be that get.Run() requires a Factory. I wonder how hard it would be to change get to use RESTClientGetter instread of Factory...
I attempted multiple times but I failed all of them (because I agree that this is the correct path). get requires concrete Factory as it relies on openapi functions (and some more) inside it.
|
unrelated |
|
/cc @soltysh |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, brianpursley The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds resource completion for kubectl debug command.
Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#1711
Special notes for your reviewer:
Does this PR introduce a user-facing change?