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 shell completion code for fish #5876

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

tiansuo114
Copy link
Contributor

@tiansuo114 tiansuo114 commented Nov 25, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add support for code completion capabilities for Fish and PowerShell terminals.
Which issue(s) this PR fixes:
Part of #5477

Special notes for your reviewer:
@zhzhuang-zju
Does this PR introduce a user-facing change?:

`karmadactl`: Add Fish shell autocompletion support for improved command-line efficiency.

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 25, 2024
@karmada-bot karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 25, 2024
@tiansuo114
Copy link
Contributor Author

While working on the code modifications for this part, I encountered some difficulties and would like to seek your assistance.

I noticed in the PR that when using Zsh and Bash as terminals, the completion functions can be loaded using the following method.

  • zsh
    source <(karmadactl completion zsh)
  • bash
    source <(karmadactl completion bash)

However, when testing with Fish and PowerShell scripts, I couldn't find a suitable way to achieve similar functionality. Currently, the scripts generated by karmadactl completion fish and karmadactl completion powershell, when saved as .fish and .ps1 files respectively and executed, work correctly for completion.

  • fish
    eval (karmadactl completion fish)
  • powershell
    karmadactl completion bash | Invoke-Expression
    . ([scriptblock]::Create((karmadactl completion bash)))
    That said, I encountered some errors when attempting to mimic the above-mentioned shortcut method to load the completion functions directly into the terminal. I would greatly appreciate your guidance on this matter.
    @zhzhuang-zju

@zhzhuang-zju
Copy link
Contributor

That said, I encountered some errors when attempting to mimic the above-mentioned shortcut method to load the completion functions directly into the terminal. I would greatly appreciate your guidance on this matter.
@zhzhuang-zju

What is the specific error?

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 47.37%. Comparing base (88c3e44) to head (ba2a073).
Report is 60 commits behind head on master.

Files with missing lines Patch % Lines
pkg/karmadactl/completion/completion.go 0.00% 10 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5876      +/-   ##
==========================================
+ Coverage   46.20%   47.37%   +1.17%     
==========================================
  Files         663      663              
  Lines       54580    54763     +183     
==========================================
+ Hits        25218    25945     +727     
+ Misses      27739    27127     -612     
- Partials     1623     1691      +68     
Flag Coverage Δ
unittests 47.37% <0.00%> (+1.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tiansuo114
Copy link
Contributor Author

tiansuo114 commented Nov 25, 2024

That said, I encountered some errors when attempting to mimic the above-mentioned shortcut method to load the completion functions directly into the terminal. I would greatly appreciate your guidance on this matter.
@zhzhuang-zju

What is the specific error?

  • karmadactl completion bash | Invoke-Expression

When I use the commands eval (karmadactl completion fish) and . ([scriptblock]::Create((karmadactl completion bash))), no error messages are displayed, but the autocomplete functionality does not take effect in the shell. It's as if the commands execute without any actual impact.

However, when I use the command karmadactl completion bash | Invoke-Expression, the following errors occur:

Invoke-Expression: Cannot bind argument to parameter 'Command' because it is an empty string.
Invoke-Expression: Cannot bind argument to parameter 'Command' because it is an empty string.
Invoke-Expression: Cannot bind argument to parameter 'Command' because it is an empty string.
Invoke-Expression: An expression was expected after '('.

All these issues happen even though saving the script outputted by the command into a file and then executing it directly in the terminal or using source xxx.fish enables the autocomplete functionality successfully.

@zhzhuang-zju
Copy link
Contributor

@tiansuo114 Thanks for your clear description. "Karmadactl completion bash" is used to generate the karmadactl completion script for Bash. The completion script depends on bash-completion, which means that you have to install this software first (you can test if you have bash-completion already installed by running type _init_completion). More info can refer to https://kubernetes.io/docs/tasks/tools/install-kubectl-linux/#install-bash-completion.

But why would you test "Karmadactl completion bash"? If want to support PowerShell, the command to generate the script should be karmadactl completion powershell >> $PROFILE.

BTW, it is necessary to ensure the selection of the correct karmadactl binary for the test architecture. Given that our current release only provides builds for Linux and Darwin OS, it seems unnecessary for karmadactl to support PowerShell. WDYT?

@tiansuo114
Copy link
Contributor Author

tiansuo114 commented Nov 27, 2024

@tiansuo114 Thanks for your clear description. "Karmadactl completion bash" is used to generate the karmadactl completion script for Bash. The completion script depends on bash-completion, which means that you have to install this software first (you can test if you have bash-completion already installed by running type _init_completion). More info can refer to https://kubernetes.io/docs/tasks/tools/install-kubectl-linux/#install-bash-completion.

But why would you test "Karmadactl completion bash"? If want to support PowerShell, the command to generate the script should be karmadactl completion powershell >> $PROFILE.

BTW, it is necessary to ensure the selection of the correct karmadactl binary for the test architecture. Given that our current release only provides builds for Linux and Darwin OS, it seems unnecessary for karmadactl to support PowerShell. WDYT?

Yes, this was indeed a mistake on my part. During actual testing, I did use the command karmadactl completion powershell | Invoke-Expression, but I mistakenly input it incorrectly earlier. My apologies for that. The method you provided successfully resolved the issue, and now I can correctly enable the code completion functionality using the karmadactl completion powershell >> $PROFILE command😄.

Regarding whether it is necessary to provide a completion method for PowerShell, I share your perspective. I also believe that for a tool currently designed only for Linux and Darwin OS, providing a PowerShell completion method is unnecessary and redundant.

At this point, I think most of the issues have been resolved. The only remaining issue pertains to the fish terminal. It seems that the eval command cannot properly load the script generated by karmadactl completion fish for command completion in the terminal. I noticed that the fish official documentation encourages placing such completion scripts in the ~/.config/fish/completions directory. Do you think using this approach as an example in the operation guide would feel somewhat inconsistent with the examples provided for zsh and bash?

@zhzhuang-zju
Copy link
Contributor

It seems that the eval command cannot properly load the script generated by karmadactl completion fish for command completion in the terminal.

How about karmadactl completion fish | source? Will this command take effect?

@tiansuo114
Copy link
Contributor Author

It seems that the eval command cannot properly load the script generated by karmadactl completion fish for command completion in the terminal.

How about karmadactl completion fish | source? Will this command take effect?

Thank you very much! This approach allows the completion scripts for the Fish terminal to load correctly. I will add the hints in the commands, remove the PowerShell-related code, and then push the changes.

@zhzhuang-zju
Copy link
Contributor

Glad to hear that! As for hints, I think we can take a page from kubectl, here's the section on fish from the kubectl completion --help

 # Load the kubectl completion code for fish[2] into the current shell
  kubectl completion fish | source
  # To load completions for each session, execute once:
  kubectl completion fish > ~/.config/fish/completions/kubectl.fish

@tiansuo114
Copy link
Contributor Author

Glad to hear that! As for hints, I think we can take a page from kubectl, here's the section on fish from the kubectl completion --help

 # Load the kubectl completion code for fish[2] into the current shell
  kubectl completion fish | source
  # To load completions for each session, execute once:
  kubectl completion fish > ~/.config/fish/completions/kubectl.fish

Ok, I changed the relevant part, now how does the code look?

@zhzhuang-zju
Copy link
Contributor

/assign
thanks for your work, reviewing is ongoing :)

@zhzhuang-zju
Copy link
Contributor

@tiansuo114 Some of the command descriptions need to be updated:

Output shell completion code for the specified shell (bash, zsh).
The shell code must be evaluated to provide interactive
completion of kubectl commands. This can be done by sourcing it from
the .bash_profile.

// TODO: support output shell completion code for more specified shell, like `fish` and `powershell`.

Short: "Output shell completion code for the specified shell (bash, zsh)",

others LGTM. And it would be nice if you could provide local verification

@tiansuo114
Copy link
Contributor Author

@tiansuo114 Some of the command descriptions need to be updated:

Output shell completion code for the specified shell (bash, zsh).
The shell code must be evaluated to provide interactive
completion of kubectl commands. This can be done by sourcing it from
the .bash_profile.

// TODO: support output shell completion code for more specified shell, like `fish` and `powershell`.

Short: "Output shell completion code for the specified shell (bash, zsh)",

others LGTM. And it would be nice if you could provide local verification

Okay, I will modify the related content as soon as possible. However, I have some questions regarding local validation. Can I understand it as adding something similar to what is pointed to in this link: #5533 (comment), or is it related to some other content?

@zhzhuang-zju
Copy link
Contributor

Can I understand it as adding something similar to what is pointed to in this link: #5533 (comment), or is it related to some other content?

yes, just show some use case will be fine

@tiansuo114 tiansuo114 changed the title add shell completion code for fish & powershell add shell completion code for fish Dec 3, 2024
@tiansuo114
Copy link
Contributor Author

This adds an example of code completion use cases for the fish shell, similar to the one in #5533.

For the fish shell, you first need to temporarily load the completion script into the terminal using the following command:

  • fish
    kubectl completion fish | source
    If you want to load it persistently into the terminal, you can use this command:
    kubectl completion fish > ~/.config/fish/completions/kubectl.fish

Once the script is loaded into the terminal, the completion behavior will be identical to what is described in the linked documentation.Details are as follows:

  • Autocomplete karmadactl
    By typing k and pressing the tab key, karmadactl can be autocompleted.
$ k
... karmadactl ... kubectl ...
  • Autocomplete karmadactl subcommands
    By typing karmadactl and pressing the tab key, the subcommands supported by karmadactl can be autocompleted.
$ karmadactl 
addons         -- Enable or disable a Karmada addon
annotate       -- Update the annotations on a resource
api-resources  -- Print the supported API resources on the server
  • Autocomplete flags
    Currently supports autocompletion for flags namespace, cluster, clusters, karmada-context, and operation-scope.
    The values autocompleted for operation-scope vary depending on the command. For example, the get command supports operation-scope values of karmada, members, and all, so these options will be available for selection during autocompletion. However, when autocompleting the operation-scope flag for the exec command, only karmada and members will appear.
$ karmadactl exec --operation-scope 
karmada  members
$ karmadactl get --operation-scope 
all      karmada  members
  • Autocomplete resources that can be operated on by commands
    For example, for the get command:
$ karmadactl get 
apiservices.apiregistration.k8s.io                            endpoints                                                     nodes.metrics.k8s.io                                          resourceregistries.search.karmada.io                        
...
$ karmadactl get pods 
nginx  pod1 

Highlights

  • The autocompletion content varies depending on the command. For example, the operation-scope flag. This not only helps complete the content but also assists users in entering the correct values.
  • When autocompleting resources for commands, it takes into account the flags operation-scope, cluster, and clusters.

Using the get command as an example:

  • karmadactl get pods will autocomplete the list of pods in the Karmada control plane.
  • karmadactl get pods --operation-scope all will autocomplete the list of pods in the Karmada control plane and all member clusters.
  • karmadactl get pods --operation-scope members will autocomplete the list of pods in all member clusters.
  • karmadactl get pods --operation-scope members --clusters member1,member2 will autocomplete the list of pods in member clusters member1 and member2.

@zhzhuang-zju
Copy link
Contributor

@tiansuo114 Good job~ just to confirm, are these commands executed in the fish shell?

@tiansuo114
Copy link
Contributor Author

@tiansuo114 Good job~ just to confirm, are these commands executed in the fish shell?

Yes, my testing environment is in an Ubuntu virtual machine on Windows, and I am using the fish shell for testing.

@zhzhuang-zju
Copy link
Contributor

Yes, my testing environment is in an Ubuntu virtual machine on Windows, and I am using the fish shell for testing.

/lgtm
I've found a previously descriptive error, so fix that along with it if you can, or we can submit a separate pr to fix it.

completion of kubectl commands. This can be done by sourcing it from

It should be completion of %[1]s commands.

Long: completionLong,

should be Long: fmt.Sprintf(completionLong, parentCommand),

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2024
@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2024
@tiansuo114
Copy link
Contributor Author

Yes, my testing environment is in an Ubuntu virtual machine on Windows, and I am using the fish shell for testing.

/lgtm I've found a previously descriptive error, so fix that along with it if you can, or we can submit a separate pr to fix it.

completion of kubectl commands. This can be done by sourcing it from

It should be completion of %[1]s commands.

Long: completionLong,

should be Long: fmt.Sprintf(completionLong, parentCommand),

Ok, now these two errors have been modified😊

The shell code must be evaluated to provide interactive
completion of kubectl commands. This can be done by sourcing it from
completion of %[1]s commands This can be done by sourcing it from
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
completion of %[1]s commands This can be done by sourcing it from
completion of %[1]s commands. This can be done by sourcing it from

The . was accidentally deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the error here has been fixed

Signed-off-by: tiansuo114 <[email protected]>

11

Signed-off-by: tiansuo114 <[email protected]>
@zhzhuang-zju
Copy link
Contributor

/lgtm
cc @RainbowMango
BTW, can you add the release note?

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2024
@tiansuo114
Copy link
Contributor Author

/lgtm cc @RainbowMango BTW, can you add the release note?

Sure, but I have some questions. I'm not sure what the release notes should include. Could you provide an example? Also, where should I add the release notes?

@zhzhuang-zju
Copy link
Contributor

I'm not sure what the release notes should include.

something like karmadactl: support output shell completion code for fish shell

where should I add the release notes?

image

@tiansuo114
Copy link
Contributor Author

I'm not sure what the release notes should include.

something like karmadactl: support output shell completion code for fish shell

where should I add the release notes?

image

Ok, relevant modifications have been completed

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2024
@karmada-bot karmada-bot merged commit f0e060d into karmada-io:master Dec 9, 2024
18 checks passed
@RainbowMango RainbowMango added this to the v1.13 milestone Dec 9, 2024
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants