8000 Docker cli slows bash init by guss77 · Pull Request #4505 · docker/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Docker cli slows bash init #4505

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 23, 2023
Merged

Docker cli slows bash init #4505

merged 1 commit into from
Aug 23, 2023

Conversation

guss77
Copy link
Contributor
@guss77 guss77 commented Aug 21, 2023 •
edited by thaJeztah
Loading

- What I did

Instead of caching the docker plugins paths during bash init, put it in a function to be called as needed.

- How I did it

This is a very simply patch that should be self evident.

- How to verify it

Check that it doesn't affect completion results with plugins installed

- Description for the changelog

  • Lookup plugins path for completion only on demand instead of caching it during bash initialization (which may be slow).

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

  ʌ___ʌ
  (O.O)
  /)__)
--''--''--

@guss77 guss77 requested a review from albers as a code owner August 21, 2023 16:50
@guss77 guss77 mentioned this pull request Aug 21, 2023
Closed
3 tasks
cpuguy83
cpuguy83 approved these changes Aug 21, 2023
View reviewed changes
Copy link
Collaborator
@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

I think the CLI could still improve here because it looks like it is making an API call to the _ping endpoint, but lazily evaluating the plugin path looks like the right thing to do here.

@cpuguy83
Copy link
Collaborator

This needs a DCO sign-off.

git commit --amend -s --no-edit && git push -f origin docker-cli-slows-bash-init

@vvoland vvoland added this to the 25.0.0 milestone Aug 22, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #4505 (ff94163) into master (cdabfa2) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #4505   +/-   ##
=======================================
  Coverage   59.38%   59.38%           
=======================================
  Files         288      288           
  Lines       24783    24783           
=======================================
  Hits        14717    14717           
  Misses       9179     9179           
  Partials      887      887

@thaJeztah
Copy link
Member

I think the CLI could still improve here because it looks like it is making an API call to the _ping endpoint

Oh! I see where that one comes from; needs a similar fix as 006c946 (#3904)

Opening a PR

@vvoland
Copy link
Collaborator
vvoland commented Aug 22, 2023

One complain from the shellcheck:

#11 5.639 In contrib/completion/bash/docker line 1147:
#11 5.639 	echo ${DOCKER_PLUGINS_PATH//:/ }
#11 5.639              ^-------------------------^ SC2086: Double quote to prevent globbing and word splitting.
#11 5.639 
#11 5.639 Did you mean: 
#11 5.639 	echo "${DOCKER_PLUGINS_PATH//:/ }"
#11 5.639

@thaJeztah thaJeztah mentioned this pull request Aug 22, 2023
Merged
@thaJeztah
Copy link
Member

This was referenced Aug 22, 2023
Open
Merged
Merged
@guss77
Copy link
Contributor Author
guss77 commented Aug 22, 2023

One complain from the shellcheck:
#11 5.639 ^-------------------------^ SC2086: Double quote to prevent globbing and word splitting.

I understand the general sentiment, but it is not correct generally and specifically in this case it is wrong:

  1. It doesn't actually matter: I'm echoing the result and whoever receives it is going to see multiple words regardless.
  2. The entire reason for converting : to spaces is to cause word spitting, which is then used in for plugin_path in $(__docker_plugins_path); do to iterate over the words.

I can change it to have the double quotes just to silence shellcheck, but that is just pasting over the real problem, which is shellcheck being over enthusiastic about recommendations that are not universally correct.

albers
albers requested changes Aug 22, 2023
View reviewed changes
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.

Thanks very much for the contribution.
Functionally LGTM, but I'd like a naming change, see comment.

@thaJeztah
Copy link
Member

For the linting error (assuming the current code is technically correct (I'm always confused by quotes in bash)); I think it'd be ok to add a # shellcheck disable=SC2086 in the code-block.

(unless there's another option that's both "good' and keeps the linter at bay of course)

@vvoland
Copy link
Collaborator
vvoland commented Aug 23, 2023

I don't think shellcheck is being completely incorrect here - the ${DOCKER_PLUGINS_PATH//:/ } is intended to be passed as a STRING argument of echo (message to be printed) and not as a whole command line to the echo.

This won't affect the word-split that we want to happen in thefor because we don't have quotes here (and shellcheck doesn't force us to change that):

for plugin_path in $(__docker_plugins_path); do

@guss77
Copy link
Contributor Author
guss77 commented Aug 23, 2023

I don't think shellcheck is being completely incorrect here - the ${DOCKER_PLUGINS_PATH//:/ } is intended to be passed as a STRING argument of echo (message to be printed) and not as a whole command line to the echo.

OK

vvoland
vvoland approved these changes Aug 23, 2023
View reviewed changes
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.

LGTM. Could you also squash your commits into one please?

@guss77
Stop slowing bash init by caching plugins path slowly
1da67be 
Fixes issue #3889 by only loading docker plugins path when needed: if it is fast enough than it shouldn't be a problem to do this on demand; OTOH if it is slow then we shouldn't do this during *every* bash session initialization, regardless if docker completion will be needed or not.

Signed-off-by: Oded Arbel <oded@geek.co.il>
@guss77
Copy link
Contributor Author
guss77 commented Aug 23, 2023

LGTM. Could you also squash your commits into one please?

Can do.

albers
albers approved these changes Aug 23, 2023
View reviewed changes
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.
I'm looking forward to this change becoming GA.

thaJeztah
thaJeztah approved these changes Aug 23, 2023
View reviewed changes
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, thank you!

@thaJeztah thaJeztah merged commit 3d11292 into docker:master Aug 23, 2023
@thaJeztah thaJeztah mentioned this pull request Aug 23, 2023
Merged
This was referenced Aug 23, 2023
Merged
Merged
@thaJeztah
Copy link
Member

Opened backports:

zzJinux added a commit to zzJinux/dotfiles that referenced this pull request Mar 9, 2024
@zzJinux
docker bash completion bug is resolved
a35d5df 
docker/cli#4505
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@albers albers albers approved these changes

@vvoland vvoland vvoland approved these changes

Assignees
No one assigned
Labels
area/completion area/performance kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.

Bash completion slows down bash login sessions
6 participants
@guss77 @cpuguy83 @codecov-commenter @thaJeztah @vvoland @albers
0