-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Shell completion #6442
Shell completion #6442
Conversation
Hi @Luap99. Thanks for your PR. I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
/hold |
@rhatdan @TomSweeneyRedHat please take a look :) |
2726a23
to
bcebd68
Compare
bcebd68
to
f5017cf
Compare
Ok i played with this a bit and tested the bash, zsh and fish completions. Bash and fish are working fine but zsh is not working properly (only completes static flags). There is already a PR in the cobra project which fixes this Issue so this should work once this get merged. I start adding autocompletion support for the flags |
@saschagrunert just putting this on your radar as an FYI as I know you have poked around in this area on other projects. |
@edsantiago PTAL I know you have played in this area in the past. |
a3975e8
to
d651b53
Compare
I started adding completion for flags.
I will edit this post to keep the table up to date with my progress. |
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.
Am only partway through review. Meant to send the spelling correction below as a single comment, but github got confused with your push --force.
I love the idea of this, and what I've seen of the implementation so far (but it will take me many more hours to complete review). If that zsh PR ever gets merged, then vendored in here, I would love to replace my crufty hand-written zsh completion. My first showstopper question is: how are you testing this? I fired up a bash shell and: $ source <(./bin/podman completion bash)
$ podman p<TAB> --> bash: _get_comp_words_by_ref: command not found [UPDATED TO ADD: bash-5.0.11-2.fc32] |
you have to use |
I did (try both |
ok i installed it with |
Solved: you need to run this first: $ source /usr/share/bash-completion/bash_completion |
cmd/podman/completion/completion.go
Outdated
// - "f29/httpd:latest" | ||
// - "f29/httpd" | ||
// - "httpd:latest" | ||
// - "httpd" |
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 would strongly encourage you to add image IDs: not all images have names. (You should probably check for that)
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.
ok i will include ids would you say i should show them only if 2 more chars are typed like the others?
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 all image IDs should be listed, honestly: there are often cases where intermediate images have no tags, and someone running podman rmi <TAB>
might want to see the full list of both IDs and tags. But this is just an opinion based on gut feel, and I don't have any evidence to support it. I think it would be OK to do what you think is best, and add a big FIXME comment saying that this needs UX testing. @mheon @rhatdan @TomSweeneyRedHat WDYT?
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'm a little worried that printing all IDs could get overwhelming, it's easy to accumulate a lot of dangling images by using podman build
. I would expect us to offer tab completion once a few characters are typed, though.
022f67a
to
6ec3988
Compare
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.
This is ready for review.
Please see this #6442 (comment) if you want to test it.
2ee38a8
to
e4cc230
Compare
I've been testing this out all afternoon, and it's just beautiful work. I'd really like to see this merged before the tree changes and @Luap99 needs to rebase yet again. @containers/podman-maintainers can we make an effort to review and merge this? |
/lgtm |
Thought I'd commented on this. |
This is only temporary until the cobra following PRs are merged: - PR#1258 Custom completion handle multiple shorhand flags together - PR#1249 Fix fish handling of "ShellCompDirectiveNoSpace" and file completion - PR#1213 Fix zsh completion handling of nospace and file completion - PR#1146 Bash completion V2 with completion descriptions Signed-off-by: Paul Holzinger <[email protected]>
Allow automatic generation for shell completion scripts with the internal cobra functions (requires v1.0.0+). This should replace the handwritten completion scripts and even adds support for fish. With this approach it is less likley that completions and code are out of sync. We can now create the scripts with - podman completion bash - podman completion zsh - podman completion fish To test the completion run: source <(podman completion bash) The same works for podman-remote and podman --remote and it will complete your remote containers/images with the correct endpoints values from --url/--connection. The completion logic is written in go and provided by the cobra library. The completion functions lives in `cmd/podman/completion/completion.go`. The unit test at cmd/podman/shell_completion_test.go checks if each command and flag has an autocompletion function set. This prevents that commands and flags have no shell completion set. This commit does not replace the current autocompletion scripts. Closes containers#6440 Signed-off-by: Paul Holzinger <[email protected]>
Add a new make target (completion) to generate the shell completion scripts. This will generate the scripts for bash, zsh and fish for both podman and podman-remote with `podman completion`. The scripts are put into the completions directory and can be installed system wide with `sudo make install.completions`. This commit replaces the current handwritten scripts for bash and zsh. The `validate.completion` target has been adjusted to make sure nobody edits these scripts directly. Signed-off-by: Paul Holzinger <[email protected]>
e4cc230
to
ae38166
Compare
Rebased with the new doc format since this would have blocked CI post-merge otherwise. |
@Luap99 I don't think it would've blocked, since github would merge on top of master (i.e. rebasing for you) but thanks. I didn't have a strong urge to repeat my testing this morning, and yesterday was a nice quiet PR day, so I just diff'ed against the previous submission: $ git cdif pr/6442{-rev05,} ...and this is the only change I see which does not look like one of yesterday's PRs: diff --git a/cmd/podman/completion/completion.go b/cmd/podman/completion/completion.go
index 71d07082c..ead8d1f05 100644
--- a/cmd/podman/completion/completion.go
+++ b/cmd/podman/completion/completion.go
@@ -87,7 +87,7 @@ func completion(cmd *cobra.Command, args []string) error {
}
_, err = io.WriteString(w, fmt.Sprintf(
- "# This file is generated with %q; see: podman-completion(1)\n", cmd.CommandPath(),
+ "\n# This file is generated with %q; see: podman-completion(1)\n", cmd.CommandPath(),
))
return err
} LGTM! |
/lgtm |
Add shell completion with cobra
Allow automatic generation for shell completion scripts
with the internal cobra functions (requires v1.0.0)
This should replace the handwritten completion scripts
and even adds support for fish and powershell(if needed?)
We can now create the scripts with
to test the completion run:
source <(podman completion bash)
I added the main completion for all commands
but this still could be improved with
better custom completion functions.
see: cmd/podman/completion/completion.go
Flag completion is for now only added to
podman generate systemd --restart-policy
for now
just to show how to implement this on flags
This is still WIP
Closes #6440