#214 Change ENTRYPOINT in Dockerfile to CMD
Merged by ralph. Opened by rayson.
rayson/waiverdb change-dockerfile-entrypoint  into  master

Download 214.patch

This is not a strong opinion but I would recommend to move the command
of starting WaiverDB service in the Dockerfile from ENTRYPOINT to CMD
for pursuing the best practices, so that ENTRYPOINT can be reserved for container initialization,
like confiurating from environment variables, waiting for services,
generating keys, or reaping zombie processes.

For OpenShift users who overwrite this command, they should overwrite
the args parameter instead of command parameter after this change.

Remove the empty lines at the end of file.

So with this change, you can pass a custom command to --entrypoint for additional configuration/management without loosing the original CMD. Do I understand it correctly?

I believe one command in openshift/waiverdb-test-template.yaml will have to be changed to args then.

@lholecek Sorry I missed your comment (Seems I didn't configure my email correctly).

So with this change, you can pass a custom command to --entrypoint for additional configuration/management without loosing the original CMD. Do I understand it correctly?

Right. This will match what most upstream containers (and those from OpenShift) do.

I believe one command in openshift/waiverdb-test-template.yaml will have to be changed to args then.

Yes.

The benefits of this are not very clear to me... initialization, configuration and waits could be done in a wrapper script provided as entrypoint.

For me using entrypoint seem the right thing to do when building application container images.

Could you point to some upstream best practices arguing in the favor of using CMD instead?

@csomh

  1. Using ENTRYPOINT to start the service makes it harder for users to run a custom command inside the container. With CMD, you can just run docker run waiverdb to start the service, or docker run -ti waiverdb /bin/bash to get into bash. But for the current image, you have to run like docker run -ti --entrypoint=/bin/bash waiverdb. This is also described in the Guidance for Docker Image Authors from Project Atomic.

  2. We can set ENTRYPOINT to an init script that is designed to be transparent to users, like tini or dockerize, or a custom script that parses environment variables and generates config files, so that the users can run additional init steps and manually start the service later.

  3. Currently S2I built images are also using CMD for the default command of starting a service. Like registry.access.redhat.com/rhscl/postgresql-95-rhel7:

$ docker inspect registry.access.redhat.com/rhscl/postgresql-95-rhel7
# ...
"Cmd": [
                "run-postgresql"
            ],
# ...

For me using entrypoint seem the right thing to do when building application container images.

Huh... fwiw, this surprises me too. I had thought ENTRYPOINT was the preferred best practice.

This one has been sitting in the review queue for a while. Let me seek out a second opinion (a third or fourth opinion), and then let's aim to resolve it soon either way.

(Ralph asked me to comment here.)

+1 to this PR:

  • Using CMD instead of ENTRYPOINT is far easier solution (especially for beginners) and the most common practice in the industry. ENTRYPOINT is usually used to invoke custom commands of a selected executable, as you guys already pointed out.

  • You also correctly change the form of the command to the exec form so the gunicorn process is pid 1 instead of shell which has gunicorn process as a child -- this is the classic zombie process reaping problem.

Thanks @ralph and @ttomecek!

OK to merge :thumbsup:

:+1: here too. I think we're going to have downstream openshift config to change after merging this.

Allright, let's get this in. Thanks all!

Commit 20005838 fixes this pull-request

Pull-Request has been merged by ralph

Pull-Request has been merged by ralph

Metadata