Fix zsh autocomplete #3221

aeriksson · 2016-03-27T23:01:14Z

This fixes a few issues with the zsh autocomplete script, and brings it up to date with the current docker-compose commands.

Changes:

Uses docker-compose config instead of manually trying to find and parse the docker-compose file. This fixes a few issues:
- Now works for version 2 docker-compose files.
- Will work correctly even if the current working directory is a subdirectory of the folder with the docker-compose file.
Now has all the flags for all the docker-compose commands.
Command help texts are now in line with what docker-compose help x produces (fixed some typos and outdated/incorrect help texts).
Now passes all relevant flags when shelling out to docker-compose and docker, so autocomplete works even if --tls, --tlskey, etc are set.
Fixed issue where autocomplete for build, pull would only show some services.
Fixed issue with filtering on already selected services when service names are substrings of one another.
All functions are now prefixed with the same number of underscores.

aeriksson · 2016-03-27T23:12:40Z

So I realized this PR conflicts with #3214 #3215 #3216 #3217 #3218. They weren't there when I set out to work on this :)

Anyway, 4201da9 contains the changes in those PRs, as well as other flag-related stuff.

If those PRs get merged first, I can rebase this branch accordingly.

dnephin · 2016-03-28T16:27:54Z

cc @sdurrheimer

sdurrheimer · 2016-03-28T18:19:07Z

@dnephin @aeriksson Yes, we should merge others PRs before considering merging this one in order to have a clear state (#3214 #3215 #3216 #3217 #3218).

I would then be able to proper review this after a rebase.

aeriksson · 2016-03-30T04:35:28Z

@sdurrheimer rebased!

sdurrheimer · 2016-03-30T06:24:13Z

contrib/completion/zsh/_docker-compose

        | sed -n -e '/^services:/,/^[^ ]/p' \
        | sed -n 's/^  //p' \
        | awk '/^[a-zA-Z0-9]/{printf "\n"};{printf $0;next;}' \
-        | awk -F: -v key=": +$1:" '$0 ~ key {print $1}' \


Can you squash that changed with @cbfe6d0 ?

ping @albers This might interest you given that we are using the same services parsing method.

@sdurrheimer Yes, that's identical with my version.
It improves readibility at the cost of one extra command invocation. I think that's OK.
I have two suggestions, though which i'll add below.
@aeriksson would you mind to adjust this function in the bash completion as well so that both completions stay aligned?

@albers sure, I'll create a bash PR too

sdurrheimer · 2016-04-29T21:03:53Z

LGTM

albers · 2016-04-30T13:19:36Z

contrib/completion/zsh/_docker-compose

+    docker-compose 2>/dev/null $compose_options "$@"
 }

 # Extracts all service names from docker-compose.yml.


Please update this comment

@albers what should the comment say?

I removed it to be in line with the bash script:
https://github.com/docker/compose/blob/master/contrib/completion/bash/docker-compose#L20-L22

I'm fine with removing the comment.

@aeriksson Can you do that change ?

@sdurrheimer not sure what the change is…

Maybe I misunderstood something, but I think @albers asked me to update that comment rather than removing it.

I then pointed out that I removed it because it was no longer needed, and there is no corresponding comment on the same function in the bash script.

I take what @albers wrote after to mean that after some deliberation, he was okay with the comment being removed.

Again, I might be wrong here, and I'd gladly add a comment if there's a need for one :)

@aeriksson The comment refers to the previous way of harvesting the service names from docker-compose.yml. The comment pointed out that the service list had some limitations: it did not take into account several other configuration files that were added over time.

Now, we use an official API for that task: docker-compose config. We let docker-compose combine all the configuration sources. But the comment was not updated and now is wrong.

There are two ways to resolve this mismatch:

change the comment to "Extracts all service names from the output of docker-compose config"

drop the comment

I favour solution 2 because the comment from solution 1 is trivial. The new code does not need a comment any more.

Yes, and solution 2 is what is implemented in this PR, so no further changes should be required, right?

@aeriksson: @albers is talking about the comment line 26/36

Oh, I see. Updated that comment to say "All services defined in docker-compose.yml" rather than "Extracts all service names from docker-compose.yml".

sdurrheimer · 2016-06-06T06:50:59Z

Hello, what is the state of this PR ?

aeriksson · 2016-06-12T21:07:03Z

AFAICT it's ready to merge.

This has the added benefit of making autocompletion work when the docker-compose config file is in a parent directory. Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

For autocomplete to work properly, we need to pass along some flags when calling docker (--host, --tls, …) and docker-compose (--file, --tls, …). Previously flags would only be passed to docker-compose, and the only flags passed were --file and --project-name. This commit makes sure that all relevant flags are passed to both docker-compose and docker. Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

Previously, the autocomplete for the build/pull commands would only add services for which build/image were the _first_ keys, respectively, in the docker-compose file. This commit fixes this, so the appropriate services are listed regardless of the order in which they appear Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

Previously, the filtering on already selected services would break when one service was a substring of another. This commit fixes that. Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

sdurrheimer · 2016-06-28T06:12:31Z

Ok LGTM

sdurrheimer · 2016-06-29T05:53:08Z

@dnephin @aanand Can we get this merged soon ? This PR fixes the version: 2 support and is kinda blocking me my upcoming PRs for new docker-compose command like bundle or push.

dnephin · 2016-06-29T14:14:49Z

LGTM

aanand · 2016-06-29T18:08:06Z

LGTM, thanks!

Fix zsh autocomplete (cherry picked from commit c0237a4) Signed-off-by: Aanand Prasad <aanand.prasad@gmail.com>

GordonTheTurtle added status/3-docs-review labels Mar 27, 2016

aeriksson force-pushed the fix-zsh-autocomplete branch from efe965b to e80d71a Compare March 27, 2016 23:07

GordonTheTurtle removed dco/no labels Mar 27, 2016

aeriksson force-pushed the fix-zsh-autocomplete branch from e80d71a to 6d5a731 Compare March 28, 2016 06:41

GordonTheTurtle added the dco/no label Mar 28, 2016

aeriksson force-pushed the fix-zsh-autocomplete branch from 6d5a731 to 6d9dec3 Compare March 28, 2016 06:42

GordonTheTurtle removed dco/no labels Mar 28, 2016

dnephin added the area/cli label Mar 28, 2016

dnephin added the kind/cleanup label Mar 28, 2016

aeriksson force-pushed the fix-zsh-autocomplete branch 2 times, most recently from 56c0e6f to 2075944 Compare March 30, 2016 04:32

GordonTheTurtle added the dco/no label Mar 30, 2016

aeriksson force-pushed the fix-zsh-autocomplete branch from 2075944 to 1bce90f Compare March 30, 2016 04:33

GordonTheTurtle added the dco/no label Mar 30, 2016

aeriksson force-pushed the fix-zsh-autocomplete branch from 1bce90f to 7e80ab4 Compare March 30, 2016 04:34

GordonTheTurtle removed dco/no labels Mar 30, 2016

sdurrheimer reviewed Mar 30, 2016
View reviewed changes

aeriksson force-pushed the fix-zsh-autocomplete branch from 15c1c4f to 2211d2f Compare April 29, 2016 03:00

GordonTheTurtle removed dco/no labels Apr 29, 2016

albers reviewed Apr 30, 2016
View reviewed changes

aeriksson and others added 13 commits June 27, 2016 11:00

fix broken zsh autocomplete for version 2 docker-compose files

cbb44b1

This has the added benefit of making autocompletion work when the docker-compose config file is in a parent directory. Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: add missing docker-compose base flags

b3d9652

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: add missing 'remove-orphans' flag for 'up' and 'down'

73a1b60

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: fix incorrect flag exclusions for 'create' and 'up'

eb10f41

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: add 'build' flag for 'create' and 'up'

8d2fbe3

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: bring flag help texts up to date

1b5a94f

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: use two underscores for all function names

97ba14c

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: fix issue when filtering on already selected services

612d263

Previously, the filtering on already selected services would break when one service was a substring of another. This commit fixes that. Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: replace use of sed with cut

0058b4b

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: break out duplicated flag messages into variables

b3d4e9c

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

zsh autocomplete: update misleading comment

c3247e7

Signed-off-by: Andre Eriksson <aepubemail@gmail.com>

aeriksson force-pushed the fix-zsh-autocomplete branch from 2211d2f to c3247e7 Compare June 27, 2016 18:08

aanand merged commit c0237a4 into docker:master Jun 29, 2016

aanand added status/5-cherry-pick and removed status/3-docs-review labels Jun 29, 2016

aanand added this to the 1.8.0 milestone Jun 29, 2016

aanand added a commit to aanand/fig that referenced this pull request Jul 6, 2016

Merge pull request docker#3221 from aeriksson/fix-zsh-autocomplete

44e82ed

Fix zsh autocomplete (cherry picked from commit c0237a4) Signed-off-by: Aanand Prasad <aanand.prasad@gmail.com>

Fix zsh autocomplete #3221

Fix zsh autocomplete #3221

Uh oh!

Conversation

aeriksson commented Mar 27, 2016

Uh oh!

aeriksson commented Mar 27, 2016

Uh oh!

dnephin commented Mar 28, 2016

Uh oh!

sdurrheimer commented Mar 28, 2016

Uh oh!

aeriksson commented Mar 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdurrheimer commented Apr 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdurrheimer commented Jun 6, 2016

Uh oh!

aeriksson commented Jun 12, 2016

Uh oh!

sdurrheimer commented Jun 28, 2016

Uh oh!

sdurrheimer commented Jun 29, 2016

Uh oh!

dnephin commented Jun 29, 2016

Uh oh!

aanand commented Jun 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants