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.

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

docker: Add proper entrypoint #531

Closed
wants to merge 1 commit into from
+21 −2
Closed

docker: Add proper entrypoint #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Dockerfile.alpine
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,5 @@ COPY --from=go-builder /build/docker-gen /usr/local/bin/docker-gen
# Copy the license
COPY LICENSE /usr/local/share/doc/docker-gen/

ENTRYPOINT ["/usr/local/bin/docker-gen"]
COPY "./container-entrypoint.sh" "/app/container-entrypoint.sh"
ENTRYPOINT [ "/app/container-entrypoint.sh" ]
3 changes: 2 additions & 1 deletion Dockerfile.debian
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,5 @@ COPY --from=go-builder /build/docker-gen /usr/local/bin/docker-gen
# Copy the license
COPY LICENSE /usr/local/share/doc/docker-gen/

ENTRYPOINT ["/usr/local/bin/docker-gen"]
COPY "./container-entrypoint.sh" "/app/container-entrypoint.sh"
ENTRYPOINT [ "/app/container-entrypoint.sh" ]
17 changes: 17 additions & 0 deletions app/container-entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/bin/sh

set -eu

bin='docker-gen'

# run command if it is not starting with a "-" and is an executable in PATH
if [ "${#}" -le 0 ] || \
Copy link
Collaborator

Choose a reason for hiding this comment

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

command will return 1 if the command name is the empty string, so this check isn't necessary.

[ "${1#-}" != "${1}" ] || \
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you pass -- to command, it will prevent $1 from being interpreted as an option. That would make it possible to eliminate this check. (I don't think it's a problem accidentally running a command named something like -x; such commands shouldn't exist, and if they do then running it is no worse than accidentally running any other command that was intended to be a command-line option to docker-gen.)

[ -d "${1}" ] || \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this implied by the command check?

! command -v "${1}" > '/dev/null' 2>&1; then
entrypoint='true'
fi

exec ${entrypoint:+${bin:?}} "${@}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

entrypoint should be initialized to the empty string in case it is already exported to this script's environment.

Copy link
Collaborator

@rhansen rhansen Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @buchdag that this should use set -- docker-gen "$@". This is the idiomatic way to "unshift", which makes it more readable.


exit 0
Comment on lines +16 to +17
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed; exec cannot return unless it can't execute the command, in which case exec's return value should be returned.

Suggested change
exit 0

Comment on lines +5 to +17
Copy link
Collaborator

Choose a reason for hiding this comment

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

Putting it all together, I would do something like this:

Suggested change
bin='docker-gen'
# run command if it is not starting with a "-" and is an executable in PATH
if [ "${#}" -le 0 ] || \
[ "${1#-}" != "${1}" ] || \
[ -d "${1}" ] || \
! command -v "${1}" > '/dev/null' 2>&1; then
entrypoint='true'
fi
exec ${entrypoint:+${bin:?}} "${@}"
exit 0
command -v -- "$1" >/dev/null 2>&1 || set -- docker-gen "$@"
exec "$@"

Copy link
Contributor Author

@oliv3r oliv3r Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for this. I've gone over this a lot in my mind over the year. Could you be right on all cases. Could my approach have been improved? I think the answer is yes, and you showed me that. I've adapted it for my general purpose needs as such:

# Prefix args with $bin if $1 is not a valid command
if ! command -v -- "${1:-}" > '/dev/null' 2>&1; then
	set -- "${bin:?}" "${@}"
fi
exec "${@}"

exit 0

I think it's shorter and more concise. I did use an 'if/else' structure instead of an 'or' structure as I find it's easier to read for passers by.

I use 'bin' instead of just pasting the command, as I reuse this snippet in multiple entry-point scripts, and having it re-usable is valuable :)

Also I'm keeping the exit 0 at the end; it's a security thing and should be done consistently. Not 'oh maybe I don't need it here', also, again re-usability.

Also, style-wise I follow the common best practice to always use curly-braces around variables.

And yes, I feel the comment is really needed here, this was a bit magical for me, or maybe that set -- wasn't in my arsenal and not immediately obvious.

But even so, thank you very much for this improvement! You helped me quite a bit to make my standard entrypoint even more ultimate :)