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

zsh completion: Fix completion when using docker run/exec -it shortcut #3075

Closed

Conversation

michelhe
Copy link

@michelhe michelhe commented May 2, 2021

This commit adds the -it support to any command that has both --interactive and --tty.
Before, the completions plugin did not recognize the -it option and thus the subcommands completion context break whenever -it is used.

- What I did
Added the -it option in the relevant places in the zsh plugin
- How I did it

- How to verify it
on the master branch of a oh-my-zsh installation, run:
docker run -it <TAB> - the container list does not appear.

now replace with the plugin from this branch:
docker run -it <TAB> - the container name completor kicks in.

- Description for the changelog

fix completion for the -it option

- A picture of a cute animal (not mandatory but encouraged)

image

This commit adds the -it support to any command that has both --interactive and --tty.
Before, the completions plugin did not recognize the -it option and thus the subcommands completion context break whenever -it is used.

Signed-off-by: Michel Heily <[email protected]>
@michelhe michelhe force-pushed the zsh-completion-support-it-shortcut branch from 4d6db5e to ab7ae87 Compare May 2, 2021 10:29
@codecov-commenter
Copy link

Codecov Report

Merging #3075 (ab7ae87) into master (759007c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3075   +/-   ##
=======================================
  Coverage   56.97%   56.97%           
=======================================
  Files         299      299           
  Lines       18731    18731           
=======================================
  Hits        10672    10672           
  Misses       7192     7192           
  Partials      867      867           

@@ -665,6 +665,7 @@ __docker_container_subcommand() {
"($help)--volume-driver=[Optional volume driver for the container]:volume driver:(local)"
"($help)*--volumes-from=[Mount volumes from the specified container]:volume: "
"($help -w --workdir)"{-w=,--workdir=}"[Working directory inside the container]:directory:_directories"
"($help)-it[shortcut for --interactive and --tty]"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this would be the right thing to do, as any shorthand flag can be combined in any order (e.g. both -ti or -it can be used).

The completion script already has support for "short option stacking"; https://github.com/docker/cli/blob/master/contrib/completion/zsh/_docker#L42-L49

But it's disabled by default, because enabling this feature caused some regression for other completion; see the discussion on moby/moby#17124 (comment)

Also, related; ohmyzsh/ohmyzsh#6710 and #993

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I wasn't aware of that.

We can close this PR, in the meantime I would use it locally for my setup. :)

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

Successfully merging this pull request may close these issues.

3 participants