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

Enable shell completion for podman commands. #4768

Closed
wants to merge 1 commit into from

Conversation

NevilleC
Copy link
Collaborator

@NevilleC NevilleC commented Dec 30, 2019

I added a command completion that generates
the shell scripts that would enable the shell
completion, using the cobra function GenBash/ZshCompletion.

Closes #3878

[vagrant@localhost libpod]$ podman build -f Dockerfile[tab][tab]
Dockerfile         Dockerfile.centos  Dockerfile.fedora

Signed-off-by: Neville Cain [email protected]

@openshift-ci-robot openshift-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 Dec 30, 2019
@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 30, 2019
@openshift-ci-robot
Copy link
Collaborator

Hi @NevilleC. 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 /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.

@NevilleC
Copy link
Collaborator Author

NevilleC commented Dec 30, 2019

WIP - just opening it to show I would like to take #3878.

It seems to work but it needs to be refined. Feel free to review but keep this in mind, please.

example:

[~]$ podman lo[tab][tab]
load    login   logout  logs 

I hope this is OK.

Update to come:

  1. Support bash and zsh
  2. update makefile to set-up this more easily

@rhatdan
Copy link
Member

rhatdan commented Dec 31, 2019

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 31, 2019
@NevilleC NevilleC force-pushed the nc-completion branch 2 times, most recently from 371dd76 to 27691ab Compare January 5, 2020 00:36
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M labels Jan 5, 2020
@NevilleC NevilleC changed the title WIP: Enable shell completion for podman commands. Enable shell completion for podman commands. Jan 5, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2020
@NevilleC NevilleC requested a review from rhatdan January 5, 2020 00:56
@rhatdan
Copy link
Member

rhatdan commented Jan 6, 2020

@edsantiago @mheon @TomSweeneyRedHat PTAL

@rhatdan
Copy link
Member

rhatdan commented Jan 6, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: NevilleC, rhatdan

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2020
@rhatdan
Copy link
Member

rhatdan commented Jan 6, 2020

@NevilleC Is there a way to further enhance this to add some existing options.
For existence how do we handle

$ podman --events-backend<tab>
file journal none

@mheon
Copy link
Member

mheon commented Jan 6, 2020

I'd like Ed to take a look at the zsh work before we merge.

On the whole, I really like doing this automatically, maintaining the completions manually is really easy to mess up (I can't guarantee we've always caught PRs that added flags, but no completions)

@edsantiago
Copy link
Member

I'm really sorry but there is no way I will approve this. The entire point of my zsh completion is for it to be simple, clean, and hands-off: all the completion code generates itself based on the output of podman --help. The approach in this PR is unmaintainable: it requires updated any time a podman flag or command is added or changed. I can't accept that. (But, fortunately, I am not the final word in this, and others can override my opinion if they see fit.)

@mheon
Copy link
Member

mheon commented Jan 6, 2020

We could probably automate a check during PR testing similar to how we do gofmt - run the regeneration command, fail if the git repo is unclean.

@NevilleC NevilleC force-pushed the nc-completion branch 2 times, most recently from e9f908e to 74101f0 Compare January 6, 2020 20:40
@NevilleC
Copy link
Collaborator Author

NevilleC commented Jan 6, 2020

Is there a way to further enhance this to add some existing options.
For existence how do we handle

@rhatdan Maybe, I'd need to search for this (maybe something like https://github.com/spf13/cobra/blob/master/bash_completions.md#specify-custom-flag-completion).

@edsantiago No problem, I do understand your point of view. What I liked with this approach is that the user does not really need to update the bash and zsh completion scripts manually, it would just need to use a command for it. That's what I tried to achieve here when I discovered that Cobra could generate those for us. So far I tested mainly with bash only (as it's what I am familiar with) and it did seem to work well.

Maybe we could keep this approach only for bash for the moment.

@mheon @edsantiago Let me know in which direction you want to go and I will update/close accordingly this PR.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4781) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2020
@edsantiago
Copy link
Member

For [example] how do we handle

$ podman --events-backend<tab>
file journal none

To be fair, I can't handle that in zsh right now either. If the podman --help output were tweaked something like this:

-  --events-backend string     Events backend to use
+  --events-backend string     Events backend to use: file, journald, or none

...then it would be pretty straightforward to parse that and add those as options. (Or I could just hardcode them directly into the zsh completion script. But as you might guess, I won't do that).

the user does not really need to update the bash and zsh completion scripts manually

@NevilleC the zsh completion script does not need to be touched. What I was trying to say earlier is that it autogenerates completions. When a user hits TAB, the completion script runs podman --help (possibly with a subcommand) and computes the possible completions automatically. It also has logic for handling directory completion, files, images, containers, pods. Cobra offers nothing even close to that.

Maybe we could keep this approach only for bash for the moment

I'm ok with that.

@rhatdan
Copy link
Member

rhatdan commented Jan 8, 2020

@edsantiago I like the idea of specifying the options in the help.

--events. ... (Valures: journald, file, none)

Something like:
diff.txt

@edsantiago
Copy link
Member

I don't like the manually-maintained aspect, but I love the consistency of the format. I'd love to see that in place.

I added a command completion that generates
the bash script that would enable the shell
completion, using the cobra function GenBashCompletion.

Closes containers#3878

Signed-off-by: Neville Cain <[email protected]>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2020
@NevilleC
Copy link
Collaborator Author

NevilleC commented Jan 9, 2020

/retest

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4817) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2020
@openshift-ci-robot
Copy link
Collaborator

@NevilleC: PR needs rebase.

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.

@NevilleC
Copy link
Collaborator Author

@rhatdan @edsantiago @mheon shall I rebase or close this MR as I am not sure it is something wanted anymore? Please let me know.

@edsantiago
Copy link
Member

@NevilleC I've looked at the current bash completion, and at the new cobra-generated one, and I lean strongly toward keeping the existing one for now, with the hope that some bashcompletionese speaker will be able to bring in the figure-things-out-automatically --help code from zsh. Reasoning: the cobra code is great for keeping every single new option up-to-date; but the existing code is carefully tuned for humans: it autocompletes images, containers, even capabilities. The latter is much friendlier and (IMO) more important than missing some options. But this is just one person's opinion.

@mheon
Copy link
Member

mheon commented Jan 13, 2020

The loss of autocomplete for container/image names could be a dealbreaker. I don't know if that would be supported in this configuration.

@rhatdan
Copy link
Member

rhatdan commented Jan 13, 2020

Right the big advance would be a way to merge the two together. IE Have a library of calls that exists for greater expansion, perhaps in a separate bash script and then have the autogenerated parts.

But this would require some real expertise and creativity in bash to make it work, I believe. Sadly if this was easy, I would figure people using cobra would be pointing it out.

@NevilleC
Copy link
Collaborator Author

Thanks for the feedback. Closing the PR.

@NevilleC NevilleC closed this Jan 13, 2020
@NevilleC NevilleC deleted the nc-completion branch January 13, 2020 21:30
@Luap99 Luap99 mentioned this pull request May 30, 2020
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bash-completion: list files for podman build -f
6 participants