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

[completion/zsh] add volume completion #2998

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

pseyfert
Copy link
Contributor

@pseyfert pseyfert commented Mar 4, 2021

This provides completion for docker run ... -v <TAB>.
This closes part of #1997.

  • the completion of docker run -it is discussed in the issue and does not require a fix (maybe a different default, but that's out of scope here)
  • the completion of docker run ... -v is addressed in this PR

As outlined here there are multiple options passed to _directories. The behaviour is that:

  • docker run -v <TAB> inserts a / immediately and will suggest directories relative to /. On my system that means docker run -v <TAB> turns into docker run -v / and the completion list contains elements such as usr,home
  • a : is inserted at the end of every suggestion. depending how the user configured their completion this means they can cycle through insertions of docker run -v /usr: docker run -v /home:and so on (for me the colon is bold face).
  • the : does not need to be removed by the user if they want to go one level higher. hitting space or / will remove the :. I.e. hitting / after the completion inserted docker run -v /usr: turns the command line into docker run -v /usr/. Continued hitting of TAB continues the game and suggests docker run -v /usr/bin:, again with a self-removing :
  • the : persists if the user types :
  • after : in the same word there is no more completion suggestions, the user needs to type the volume mount on their own.
  • after a (such as docker run -v /home or docker run -v /home:/hosthome ) the completion continues to complete arguments to docker run just like it did before this PR.

Verification was trying around on the command line with docker run -it --rm -v <stuff here> debian:testing. I think the suggestions should be absolute paths (the original draft suggested relative paths to the cwd, but docker didn't mount them).

One can probably argue if the treatment of the trailing : is optimal (self removal on space and slash. i caught myself a few times that after having docker run -it --rm -v /home: i tried to continue with /hosthome and wanting to type-v /home:/hosthome but typed /home/hosthome as the / overwrote the visual :).

not an animal, but a cake:
cake

This provides completion for `docker run ... -v <TAB>`.
This closes (part of) docker#1997.

Signed-off-by: Paul Seyfert <pseyfert.mathphys@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #2998 (35b42ef) into master (70a0015) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2998   +/-   ##
=======================================
  Coverage   57.06%   57.06%           
=======================================
  Files         299      299           
  Lines       18683    18683           
=======================================
  Hits        10662    10662           
  Misses       7155     7155           
  Partials      866      866           

@pseyfert
Copy link
Contributor Author

Any maintainer out there willing to review / approve? ( @thaJeztah , tagging you as you appear in the other completion related MRs)

Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

Looks good to me.
However, I think that in the long run we would like to get rid of the manually maintained completions scripts and continue with the approach started by @ndeloof with #3429 (using Cobra's completion generation feature).
So probably it would be good to take a look how to implement this completion with the Cobra.

@pseyfert
Copy link
Contributor Author

@vvoland thanks for the feedback. I appreciate the effort going for a generated completion feature rather than the manually maintained function. That said, shall we still try to merge the MR here? It improves the completion file while it's still there. And admittedly I don't (yet?) find my way around the cobra completion, so it will take a bit until I can contribute to that.

@thaJeztah
Copy link
Member

The only thing I was wondering is if it should complete (named) volumes, and if so, how we could make it either complete those, or local paths (for bind-mounts). Not exactly sure what we currently do for the Bash completion (maybe we don't do it there either)

@thaJeztah thaJeztah closed this Oct 27, 2022
@thaJeztah thaJeztah reopened this Oct 27, 2022
@thaJeztah
Copy link
Member

(close/reopen to trigger CI)

@thaJeztah thaJeztah added this to the 23.0.0 milestone Jan 13, 2023
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 1af9f22 into docker:master Jan 13, 2023
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.

4 participants