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.

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

Add bash and zsh completion support #384

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

concaf
Copy link
Contributor

@concaf concaf commented Mar 15, 2018

This commit adds support for auto completion for bash and zsh
shells. A new root level command called "completion" has been
introduced, and the user can get the auto completion code by
running ark completion bash/zsh.

For bash completion, the built-in GenBashCompletion() from cobra
has been used, but for zsh, the built-in GenZshCompletion() is
known to cause issues. The workaround has been copied from zsh
completion code of kubectl.

Fixes #381

@concaf concaf force-pushed the add-bash-zsh-completion branch from 18cd7df to 7a9c03d Compare March 15, 2018 17:53
@ncdc
Copy link
Contributor

ncdc commented Mar 15, 2018

What are the issues with GenZshCompletion?

@@ -0,0 +1,207 @@
/*
Copyright 2017 the Heptio Ark contributors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}

func genZshCompletion(out io.Writer, ark *cobra.Command) {
// The following code has been copied from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to move this to third_party and include the license header from the original file. Let me double check and get back to you later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @ncdc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can find a way to put this in third_party under kubernetes/pkg/kubectl/cmd/completion.go along with the original license and a line saying that modifications are © Heptio Ark Contributors, that'd probably be best.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually included both the licences in the new file in third_party, PTAL

@nrb
Copy link
Contributor

nrb commented Mar 15, 2018

Thanks for this!

I've verified it works on zsh, but the CLI docs also need updating. You can do so by running hack/update-generated-docs.sh and adding the modified file.

@concaf
Copy link
Contributor Author

concaf commented Mar 16, 2018

What are the issues with GenZshCompletion?

I don't know that exact issue since I don't use zsh, but it doesn't work for me 🤷‍♂️

$ source <(ark completion zsh)
_arguments:comparguments:319: can only be called from completion function
_arguments:comparguments:319: can only be called from completion function

Most of the projects have the same workaround.

@concaf concaf force-pushed the add-bash-zsh-completion branch 2 times, most recently from 0cd52d9 to 3c7e162 Compare March 16, 2018 06:19
@concaf
Copy link
Contributor Author

concaf commented Mar 16, 2018

You can do so by running hack/update-generated-docs.sh and adding the modified file.

@nrb done 🎉

"fmt"
"os"

kubectlCmd "github.com/heptio/ark/third_party/kubernetes/pkg/kubectl/cmd"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't camel case import aliases. I'd use kubectlcmd or kcmd. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to kubectlcmd

/*
The original code is from https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/completion.go -

Copyright 2016 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the modifications line below this one:

Modifications copyright 2018 the Heptio Ark contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

limitations under the License.
*/

/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this stanza

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@ncdc
Copy link
Contributor

ncdc commented Apr 3, 2018

@containscafeine sorry for the delay - just got back from PTO!

This commit adds support for auto completion for bash and zsh
shells. A new root level command called "completion" has been
introduced, and the user can get the auto completion code by
running `ark completion bash/zsh`.

For bash completion, the built-in GenBashCompletion() from cobra
has been used, but for zsh, the built-in GenZshCompletion() is
known to cause issues. The workaround has been copied from zsh
completion code of kubectl.

Signed-off-by: Shubham <shubham@linux.com>
@concaf concaf force-pushed the add-bash-zsh-completion branch from 8815537 to 7f3e881 Compare April 12, 2018 07:56
@concaf
Copy link
Contributor Author

concaf commented Apr 12, 2018

@ncdc made the changes, PTAL

@ncdc
Copy link
Contributor

ncdc commented Apr 12, 2018

Thanks @containscafeine!!!

@ncdc ncdc merged commit e78c892 into vmware-tanzu:master Apr 12, 2018
@marioaparcero
Copy link

source <(velero completion zsh) > ~/.zshrc (Not working for me) I tested it in Linux Mint.

@nrb
Copy link
Contributor

nrb commented May 7, 2021

@marioaparcero Please file a new issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants