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/zsh completion command #1437

Merged

Conversation

bharathi-tenneti
Copy link
Contributor

Description:
Add completion command to enable Auto bash/zsh completion.
Motivation:
To have kubebuilder cli auto completion enabled.
Closes: #1346

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 19, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @bharathi-tenneti!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @bharathi-tenneti. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2020
go.mod Outdated
k8s.io/api v0.17.4
k8s.io/apimachinery v0.17.4
k8s.io/client-go v11.0.0+incompatible
sigs.k8s.io/controller-runtime v0.5.1
Copy link
Member

@camilamacedo86 camilamacedo86 Mar 19, 2020

Choose a reason for hiding this comment

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

Upgrade the API's are not part of the scope of his PR.
Could you please revert?

@@ -10,7 +10,7 @@ spec:
spec:
containers:
- name: kube-rbac-proxy
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.4.1
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.5.0
Copy link
Member

Choose a reason for hiding this comment

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

Could you please rebase your branch with the master?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2020
func newCompletionCmd() *cobra.Command {
completionCmd := &cobra.Command{
Use: "completion",
Short: "Generators for shell completions",
Copy link
Member

@camilamacedo86 camilamacedo86 Mar 20, 2020

Choose a reason for hiding this comment

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

WDYT about we add the Lond desc and the Examples? See an example of this impl:

Long: `Initialize a new project including vendor/ directory and Go package directories.
?

Also, I think that the long desc could describe its purpose as to how to use it.

func newCompletionCmd() *cobra.Command {
completionCmd := &cobra.Command{
Use: "completion",
Short: "Generators for shell completions",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Short: "Generators for shell completions",
Short: "Generators for shell completions",
Long:`Output shell completion code for the specified shell (bash or zsh). The shell code must be evaluated to provide interactive completion of kubebuilder commands. This can be done by sourcing it from the .bash_profile.
Detailed instructions on how to do this are available here:
<add a link for the kubebuilder docs with the further information, note that it should similar to https://kubernetes.io/docs/tasks/tools/install-kubectl/#enabling-shell-autocompletion>`

Short: "Generate bash completions",
RunE: func(cmd *cobra.Command, cmdArgs []string) error {
return cmd.Root().GenBashCompletion(os.Stdout)
},
Copy link
Member

Choose a reason for hiding this comment

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

Also, we need to add the Examples:

Example: `# Run the command to generate the bash completion and create the file
$kubebuilder completion bash >> ~/.kubebuilder

TODO: Add all the steps required to set up it. 


"

Copy link
Contributor

Choose a reason for hiding this comment

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

$ kubebuilder completion bash >> ~/.bashrc

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
},
},
Example: `To load completion run:
$ . <(kubebuilder completion bash)
To configure your bash shell to load completions for each session add to your bashrc:
$ echo -e "\n. <(kubebuilder completion bash)" >> ~/.bashrc
`,

@estroz
Copy link
Contributor

estroz commented Mar 21, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 21, 2020
@bharathi-tenneti bharathi-tenneti changed the title Add bash/zsh completion command <WIP> Add bash/zsh completion command Mar 25, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2020
@@ -0,0 +1,53 @@
# Enabling shell auto completion
Copy link
Member

Choose a reason for hiding this comment

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

I am not able to check it in the Reference category in the preview. Could you please check?

Screenshot 2020-03-26 at 11 21 36


`echo BASH_VERSION`

#### To upgrade, you may use homebrew:
Copy link
Member

Choose a reason for hiding this comment

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

If these are the steps then, I do not think that we should use ### since it is for section titles.

Copy link
Member

Choose a reason for hiding this comment

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

See an example of the layout applied in another place: https://deploy-preview-1437--kubebuilder.netlify.com/reference/kind.html#cheetsheet

RunE: func(cmd *cobra.Command, cmdArgs []string) error {
return cmd.Root().GenZshCompletion(os.Stdout)
},
}
Copy link
Member

Choose a reason for hiding this comment

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

Wdyt about add the examples here in the commands?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2020
</aside>

<aside class="note">
<h1>Enable shell auto completion</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<h1>Enable shell auto completion</h1>
<h1>Enabling shell autocompletion</h1>

To keep the same standard applied in the docs.

<aside class="note">
<h1>Enable shell auto completion</h1>

Optional: To enable kubebuilder shell auto completion, please refer to [this](./reference/completion.md) document.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Optional: To enable kubebuilder shell auto completion, please refer to [this](./reference/completion.md) document.
Kubebuilder provides autocompletion support for Bash and Zsh via the command `kubebuilder completion <bash|zsh>`, which can save you a lot of typing. For further information see the [completion](./reference/completion.md) document.

@@ -0,0 +1,29 @@
# Enable shell auto completion

Copy link
Member

@camilamacedo86 camilamacedo86 Mar 30, 2020

Choose a reason for hiding this comment

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

Suggested change
The Kubebuilder completion script for Bash can be generated with the command `kubebuilder completion bash` as the Kubebuilder completion script for Zsh can be generated with the command `kubebuilder completion zsh`.
Note that sourcing the completion script in your shell enables Kubebuilder autocompletion.

@@ -0,0 +1,29 @@
# Enable shell auto completion
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Enable shell auto completion
# Enabling shell autocompletion

Copy link
Member

Choose a reason for hiding this comment

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

Following the docs standard/format for titles

@@ -0,0 +1,29 @@
# Enable shell auto completion

## Prerequisites
Copy link
Member

@camilamacedo86 camilamacedo86 Mar 30, 2020

Choose a reason for hiding this comment

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

Suggested change
## Prerequisites
</aside>
<aside class="note">
<h1>Prerequisites for Bash</h1>
The completion Bash script depends on [bash-completion](https://github.com/scop/bash-completion), which means that you have to install this software first (you can test if you have bash-completion already installed. Also, ensure that your Bash version is 4.1+.
</aside>

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to talk about zsh too

Copy link
Member

Choose a reason for hiding this comment

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

@estroz Is the bash-completion a prerequisite for the zsh?

## Prerequisites

- Bash 4.1+/zsh
- bash-completion/zsh-completions v2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- bash-completion/zsh-completions v2

Copy link
Member

Choose a reason for hiding this comment

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

Info added in the above note.


## Prerequisites

- Bash 4.1+/zsh
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Bash 4.1+/zsh

Copy link
Member

Choose a reason for hiding this comment

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

Info added in the above note.


- Once installed, go ahead and add the path `/usr/local/bin/bash` in the `/etc/shells`.

`sudo echo “/usr/local/bin/bash” > /etc/shells`
Copy link
Member

Choose a reason for hiding this comment

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

HI @bharathi-tenneti and @estroz,

  • Why the sudo here is required?
  • Also, note that the bash requirements is just important for the bash one and not zsh. So, we cannot have the same pre-requirements for both. See my suggestion above
  • Are these steps valid for any OS? So how can we add generic steps if they are not?. I think we could do similar the structure of the kubtecl docs here. See: https://kubernetes.io/docs/tasks/tools/install-kubectl/#enabling-shell-autocompletion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same concern as below #1437 (comment)


- Bash 4.1+/zsh
- bash-completion/zsh-completions v2

Copy link
Member

Choose a reason for hiding this comment

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

IMO it should be :

Bash on Linux

//Add the steps

Bash on Mac

// Add the steps

Zsh

// Add the steps

Copy link
Contributor

@estroz estroz Mar 30, 2020

Choose a reason for hiding this comment

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

We should avoid discussing prereq installation steps on different systems, since "linux" has various distros (read: package managers). Perhaps a link to each package would help.

Listing the prerequisite packages and versions delegates installation (which is relatively simple in this case) to the user. By doing so we also avoid doc drift in case prereqs change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@estroz @camilamacedo86
As I could test out on macOS, I can list out steps for the same.
Any idea on how do folks go about for other distributions?
Can I leave a TODO note for others such as ""linux", and "zsh"?

Copy link
Member

@camilamacedo86 camilamacedo86 Mar 31, 2020

Choose a reason for hiding this comment

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

The problem here is that I am not sure if the same steps will work for both (Linux or Mac).

Regards the pre-requirement for bash it is the same for kubectl and for both (Linux or Mac) its docs is addressing the same repo. There has the info over how to install in each SO.

So, IMO we can move forward with:

Enabling shell autocompletion

(introduction)

Bash

(pre-requirement note)

(steps)

Then, we can improve it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@camilamacedo86
So, no changes for now. Did I understand correctly?

@mengqiy
Copy link
Member

mengqiy commented Mar 30, 2020

/approve
/hold
@camilamacedo86 @estroz Please feel free to unhold it after comments are addressed.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 30, 2020
@camilamacedo86
Copy link
Member

@bharathi-tenneti,

@mengqiy is ok to merge it after address the comments made an fix the docs.
Am I right @mengqiy? Also, please let if I understood something here.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2020
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

@bharathi-tenneti,

You can use the preview to check the changes. Just few nits are missing here for we are able to move forward. See: https://deploy-preview-1437--kubebuilder.netlify.com/reference/completion.html

  • Fix the link:

Screenshot 2020-03-31 at 21 06 09

  • Put the NOTE Follow a similar protocol for zsh completion. in an not follow its standard.
    See:

```
- Restart terminal for the changes to be reflected.

Follow a similar protocol for `zsh` completion.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Follow a similar protocol for `zsh` completion.
</aside>
<aside class="note">
<h1>Zsh</h1>
Follow a similar protocol for `zsh` completion.
</aside>

@bharathi-tenneti
Copy link
Contributor Author

/retest

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

I think that a few details still with a hall to improvements, however, we can do it in a follow-up pr and it shows to attend the requirements and is OK for others as well.

So,
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bharathi-tenneti, camilamacedo86, mengqiy

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:
  • OWNERS [camilamacedo86,mengqiy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@camilamacedo86
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 76050aa into kubernetes-sigs:master Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate CLI completions for bash/zsh
5 participants