Skip to content

Conversation

@varp
Copy link
Contributor

@varp varp commented Dec 19, 2021

Signed-off-by: Vardan Pogosian vardan.pogosyan@gmail.com

- What I did
Replaced sed invocation with Go templates invocations

- How I did it
Replaced sed invocation with Go templates invocations

- How to verify it
Install docker-compose 2.2.2 as a plugin for docker CLI on Mac OS X and try to auto-complete docker comp<tab>. It will auto-complete to docker compose

- Description for the changelog

Made sed invocation, used at COMPOSER_PLUGIN_PATH detection (for BASH completion) compatible with POSIX 1003.2 RE to properly work on OS X.

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

@varp varp requested a review from albers as a code owner December 19, 2021 15:06
@varp
Copy link
Contributor Author

varp commented Dec 20, 2021

@albers Hello, could you take some action on this?

@varp varp force-pushed the make_compose_plugin_detection_compatible_with_posix_bre branch from 6ca1804 to 3efc8d3 Compare December 22, 2021 07:25
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.

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.

Sorry, there's still one thing to do with your PR: please sign off the commit, see the failing DCO check.

@varp varp force-pushed the make_compose_plugin_detection_compatible_with_posix_bre branch from 3efc8d3 to d37fc04 Compare December 24, 2021 07:15
@varp
Copy link
Contributor Author

varp commented Dec 24, 2021

@albers done. The commit now has sign

@codecov-commenter
Copy link

codecov-commenter commented Dec 25, 2021

Codecov Report

Merging #3392 (77b1031) into master (2fe3515) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3392   +/-   ##
=======================================
  Coverage   56.35%   56.35%           
=======================================
  Files         304      304           
  Lines       26830    26830           
=======================================
  Hits        15121    15121           
  Misses      10789    10789           
  Partials      920      920           

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

@albers
Copy link
Collaborator

albers commented Dec 26, 2021

@thaJeztah PTAL

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.

Thanks @varp

change LGTM, but I left a suggestion; let me know what you (and @albers) think about that approach.

Happy to merge as-is if you don't think it's better (or if you prefer handling that as a follow-up)

}

COMPOSE_PLUGIN_PATH=$(docker info --format '{{json .ClientInfo.Plugins}}' | sed -n 's/.*"Path":"\([^"]\+docker-compose\)".*/\1/p')
COMPOSE_PLUGIN_PATH=$(docker info --format '{{json .ClientInfo.Plugins}}' | sed -n -E 's/.*"Path":"([^"]+docker-compose)".*/\1/p')
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if we could do it with --format alone, and (while the syntax is a bit awkward), looks like it's possible when combining range and if eq;

docker info --format '{{range .ClientInfo.Plugins}}{{if eq .Name "compose"}}{{.Path}}{{end}}{{end}}'
/usr/local/lib/docker/cli-plugins/docker-compose

The script would then look like;

COMPOSE_PLUGIN_PATH=$(docker info --format '{{range .ClientInfo.Plugins}}{{if eq .Name "compose"}}{{.Path}}{{end}}{{end}}')

Copy link
Member

Choose a reason for hiding this comment

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

There's one small difference; the docker info --format always prints a newline, whereas the sed variant does not; if I replace compose with nosuch (to mimic a situation where the compose plugin is not installed);

docker info --format '{{range .ClientInfo.Plugins}}{{if eq .Name "nosuch"}}{{.Path}}{{end}}{{end}}'
# prints an empty line

docker info --format '{{json .ClientInfo.Plugins}}' | sed -n -E 's/.*"Path":"([^"]+docker-nosuch)".*/\1/p'
# doesn't print an empty line

But I don't think that's a problem as the script checks if $COMPOSE_PLUGIN_PATH contains a valid path, and that still looks to work ok with that;

# for the "nosuch" plugin:

COMPOSE_PLUGIN_PATH=$(docker info --format '{{range .ClientInfo.Plugins}}{{if eq .Name "nosuch"}}{{.Path}}{{end}}{{end}}')
if [ -f "$COMPOSE_PLUGIN_PATH" ] ; then echo YES; fi

# doesn't print anything

# for the "compose" plugin:
COMPOSE_PLUGIN_PATH=$(docker info --format '{{range .ClientInfo.Plugins}}{{if eq .Name "compose"}}{{.Path}}{{end}}{{end}}')
if [ -f "$COMPOSE_PLUGIN_PATH" ] ; then echo YES; fi

# prints "YES"

Copy link
Collaborator

Choose a reason for hiding this comment

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

@thaJeztah Your suggestion has the advantage that it accesses the information in a structured way (i.e. by .Name) instead of text based operations that rely on naming conventions.
So I think we should go this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@albers Should do I bring a new PR and decline this one with the solution suggested by @thaJeztah or just force push a new commit and change the PR description and the name?
I'm asking because the PR already has approves and assigned milestone.
As a compromise, I could bring suggestions by @thaJeztah in a new PR for the next milestone (not the assigned one - 21.xx)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's best to force-push and then update the PR title to a more intention revealing wording, something like "make compose plugin detection in bash completion work on Mac OS"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@albers It's done

Signed-off-by: Vardan Pogosian <vardan.pogosyan@gmail.com>
@varp varp force-pushed the make_compose_plugin_detection_compatible_with_posix_bre branch from d37fc04 to 77b1031 Compare January 9, 2022 09:16
@varp varp changed the title compatible sed execs with POSIX 1003.2 BRE on COMPOSE_PLUGIN_PATH make compose plugin detection in bash completion work on Mac OS Jan 9, 2022
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 @varp

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!!

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