-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for bash/zsh completion #2833
Add support for bash/zsh completion #2833
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the minor comment this looks good to me, thanks for putting time and effort into this 🌷
I would like another maintainer (maybe @stefanprodan or @squaremo) to have a look at this, as I am not really familiar with the ins and outs of Cobra.
@hiddeco seems like the docker container running kubeadm to set up the e2e tests run out of memory 😕Any idea if this is a transient failure and if so could we trigger the build again? |
It is a transient error we are working on with the folks of Kind, I have rebased your PR to get rid of the commits from master that were accidentally pulled in, which has also triggered a new CI build 🤞 |
/additional-reviewer-roulette (whoever comes first) |
cmd/fluxctl/completion_cmd.go
Outdated
} | ||
|
||
return &cobra.Command{ | ||
Use: "completion shell", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to distinguish syntactically between completion
which is literal, and shell
which is user input? (or indeed, give the possible values)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense here to utilize the shells
(again), and put <>
around the list to highlight the fact that they are options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, only issue is that the Use
field is also used by the root command as a way to distinguish which of the commands need to skip port forward. So the completion command would have to get instantiated there (or come up with a different way of detecting which commands are run). Alternatively, we could change it to completion SHELL
to distinguish them, similar to kubectl
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given people are hopefully already familiar with kubectl
, I think that’s a good alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work, thank you!
(needs a rebase before it can be merged; if you don't get to it @yiannistri then someone will do it eventually :-) |
@yiannistri can you please rebase on top of |
Thank you @yiannistri! 🌻 |
Fixes #1593