Skip to content

Conversation

@ulyssessouza
Copy link
Contributor

@ulyssessouza ulyssessouza commented Jun 23, 2021

WIP: Note that this PR depends on the implementation of completions PR in docker/compose-cli to work.

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2021

Codecov Report

Merging #3158 (efba76c) into master (5f07d7d) will increase coverage by 1.50%.
The diff coverage is n/a.

❗ Current head efba76c differs from pull request most recent head 5a8d7d5. Consider uploading reports for the commit 5a8d7d5 to get more accurate results

@@            Coverage Diff             @@
##           master    #3158      +/-   ##
==========================================
+ Coverage   57.08%   58.58%   +1.50%     
==========================================
  Files         299      299              
  Lines       18756    21476    +2720     
==========================================
+ Hits        10707    12582    +1875     
- Misses       7178     7972     +794     
- Partials      871      922      +51     

@ulyssessouza ulyssessouza marked this pull request as ready for review June 29, 2021 01:18
@ulyssessouza ulyssessouza requested a review from albers as a code owner June 29, 2021 01:18
Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

I think this PR is a very good starting point to gain some experience with generated completions.
We could start with a hardcoded option to include completion for the compose plugin, as suggested in this PR.

The completion script could check the well-known install locations (/usr/libexec/docker/cli-plugins/ and ~/.docker/cli-plugins/ for the presence of the docker-compose plugin file. In case of a match, it should activate completion for the plugin.

In subsequent PRs, the completion script should be extended to provide a generic solution for client plugins.

The generated completion as implemented in this PR works quite well, but should also be improved in subsequent PRs. For example, I miss support for completion of local files (docker compose -f), and completion of service names (docker compose logs)

@thaJeztah
Copy link
Member

for #3158 (comment)

I should not that if we do want to use the above docker info to get that information, and we must do that on every request, we should have a look if we can somehow get the info without the CLI also making an API call (to the /info endpoint) 😬

I opened #3179, which should be able to detect if we're only using local information (in which case it will skip making an API connection to the /info endpoint.

Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

For consistency, please avoid unneccessary braces in variables, e.g. "${value}" should be just "$value". For the same reason please pull up the do.

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

Very well.
One last nit and a question.

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
ping @thaJeztah

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

tried it inside a container, and looks to work ok 👍

I also see spf13/cobra#1146 was merged, which looks to be improving cobra's completion scripts as well (instead of generating massive scripts)

@thaJeztah thaJeztah merged commit 72066d5 into docker:master Jul 20, 2021
@thaJeztah thaJeztah added this to the 21.xx milestone Jul 20, 2021
@ulyssessouza
Copy link
Contributor Author

Thanks a lot @albers & @thaJeztah !

@albers
Copy link
Collaborator

albers commented Jul 20, 2021

@ulyssessouza Please keep me up to date with your further work on generated completions.

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.

5 participants