Skip to content

Rename docker-entrypoint to container-entrypoint #59

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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Sebastian-Maier
Copy link

This PR addresses issue #56 by using vendor-agnostic "container" terminology instead of "Docker" (in a backwards-compatible fashion):

  • introduces container-entrypoint.sh that supports custom scripts in both /docker-custom-entrypoint.d/and /container-custom-entrypoint.d/ directories
  • adjusts docker-entrypoint.sh to print a deprecation warning and call container-entrypoint.sh
  • adjusts documentation accordingly

# echo the return code of the application
exit $return_code
echoerr "DEPRECATED: Use /container-entrypoint.sh instead of /docker-entrypoint.sh"
./container-entrypoint.sh "$@"

Choose a reason for hiding this comment

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

Suggested change
./container-entrypoint.sh "$@"
exec ./container-entrypoint.sh "$@"

This should avoid a sub-process. But I'm not entirely sure about all the security implications and if the "$@" still is required.

Copy link
Author

Choose a reason for hiding this comment

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

Tested and adjusted as proposed.

Comment on lines 58 to 68
run_custom_handler pre-default

for f in /container-entrypoint.d/*.sh; do
Copy link

@nsballmann nsballmann Jul 30, 2025

Choose a reason for hiding this comment

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

Suggested change
run_custom_handler pre-default
for f in /container-entrypoint.d/*.sh; do
run_custom_handler pre-default
for f in /docker-entrypoint.d/*.sh; do
echoerr "DEPRECATED: Use /container-entrypoint.d/ for $f"
echo "Running $f"
"$f"
done
for f in /container-entrypoint.d/*.sh; do

For additional backwards compatibility.

edit: Forgot that $f is absolut already.

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted this script accordingly: For now we also execute scripts in /docker-entrypoint.d/ but there will be only one deprecation warning when the directory exists.

Choose a reason for hiding this comment

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

Ah yeah, true, testing the directory exists is another thing I forgot. That's more defensive. One deprecation warning should be enough.

@@ -5,7 +5,7 @@ set -e
if [[ "$OPENVOXSERVER_GRAPHITE_EXPORTER_ENABLED" == "true" ]]; then
if [[ -n "$OPENVOXSERVER_GRAPHITE_HOST" && -n "$OPENVOXSERVER_GRAPHITE_PORT" ]]; then
echo "Enabling graphite exporter"
sed -e "s/GRAPHITE_HOST/$OPENVOXSERVER_GRAPHITE_HOST/" -e "s/GRAPHITE_PORT/$OPENVOXSERVER_GRAPHITE_PORT/" /docker-entrypoint.d/84-metrics.conf.tmpl > /etc/puppetlabs/puppetserver/conf.d/metrics.conf
sed -e "s/GRAPHITE_HOST/$OPENVOXSERVER_GRAPHITE_HOST/" -e "s/GRAPHITE_PORT/$OPENVOXSERVER_GRAPHITE_PORT/" /container-entrypoint.d/84-metrics.conf.tmpl > /etc/puppetlabs/puppetserver/conf.d/metrics.conf

Choose a reason for hiding this comment

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

Suggested change
sed -e "s/GRAPHITE_HOST/$OPENVOXSERVER_GRAPHITE_HOST/" -e "s/GRAPHITE_PORT/$OPENVOXSERVER_GRAPHITE_PORT/" /container-entrypoint.d/84-metrics.conf.tmpl > /etc/puppetlabs/puppetserver/conf.d/metrics.conf
my_directory="$(dirname "$0")"
sed -e "s/GRAPHITE_HOST/$OPENVOXSERVER_GRAPHITE_HOST/" -e "s/GRAPHITE_PORT/$OPENVOXSERVER_GRAPHITE_PORT/" "${my_directory}/84-metrics.conf.tmpl" > /etc/puppetlabs/puppetserver/conf.d/metrics.conf

This is just a suggestion. It's IMHO a little more elegant way to say "in the same directory like the shell script". It introduces a coreutils dependency because of dirname, though.

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted (in a slightly different manner).

Choose a reason for hiding this comment

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

Because of the readlink it now looks for "in the same directory as the file or symlinked-to file" instead of the "in the same directory as the file or the symlink itself". Thinking about this, I looks to me like your proposal is a little cleaner.

Because now one can move the script and the template file into a "library" like location and just symlink the entrypoint script into the entrypoint.d directory.

@@ -6,6 +6,6 @@ if [[ "$OPENVOXSERVER_ENABLE_ENV_CACHE_DEL_API" == true ]]; then
if [[ $(grep 'puppet-admin-api' /etc/puppetlabs/puppetserver/conf.d/auth.conf) ]]; then
echo "Admin API already set"
else
/opt/puppetlabs/puppet/bin/ruby /docker-entrypoint.d/88-add_cache_del_api_auth_rules.rb
/opt/puppetlabs/puppet/bin/ruby /container-entrypoint.d/88-add_cache_del_api_auth_rules.rb
Copy link

@nsballmann nsballmann Jul 30, 2025

Choose a reason for hiding this comment

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

Suggested change
/opt/puppetlabs/puppet/bin/ruby /container-entrypoint.d/88-add_cache_del_api_auth_rules.rb
my_directory="$(dirname "$0")"
/opt/puppetlabs/puppet/bin/ruby "${my_directory}/88-add_cache_del_api_auth_rules.rb"

Same like #59 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted (in a slightly different manner).

@@ -4,5 +4,5 @@ set -e

if [ -n "${CSR_ATTRIBUTES}" ]; then
echo "CSR Attributes: ${CSR_ATTRIBUTES}"
/opt/puppetlabs/puppet/bin/ruby /docker-entrypoint.d/89-csr_attributes.rb
/opt/puppetlabs/puppet/bin/ruby /container-entrypoint.d/89-csr_attributes.rb
Copy link

@nsballmann nsballmann Jul 30, 2025

Choose a reason for hiding this comment

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

Suggested change
/opt/puppetlabs/puppet/bin/ruby /container-entrypoint.d/89-csr_attributes.rb
my_directory="$(dirname "$0")"
/opt/puppetlabs/puppet/bin/ruby "${my_directory}/89-csr_attributes.rb"

Same like #59 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted (in a slightly different manner).

@Sebastian-Maier Sebastian-Maier force-pushed the refactor/container-entrypoint branch from 2fba4de to 8dbaf8a Compare July 31, 2025 09:16
@rwaffen
Copy link
Member

rwaffen commented Aug 1, 2025

i enabled the ci for your pr. nice work and good to see a lively discussion. keep up the good work. will review in detail later. one thing: please sign your commits :)

@Sebastian-Maier Sebastian-Maier force-pushed the refactor/container-entrypoint branch from 8dbaf8a to 577f134 Compare August 4, 2025 10:17
@Sebastian-Maier
Copy link
Author

i enabled the ci for your pr. nice work and good to see a lively discussion. keep up the good work. will review in detail later. one thing: please sign your commits :)

The commits of the PR are now signed with GPG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants