-
-
Notifications
You must be signed in to change notification settings - Fork 224
Don't pollute output when testing Postgres connection #33
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
Don't pollute output when testing Postgres connection #33
Conversation
I just noticed that this has already been fixed upstream, see https://github.com/api-platform/api-platform/blob/master/api/docker/php/docker-entrypoint.sh#L17 What we have to do is just to sync this file with the one in api-platform/api-platform. |
4852c1a
to
fa5e752
Compare
Replaced in api-platform#31
fa5e752
to
9fffd40
Compare
I restore the command I have the postgres image
|
@@ -14,7 +14,7 @@ if [ "$1" = 'php-fpm' ] || [ "$1" = 'bin/console' ]; then | |||
if [ "$APP_ENV" != 'prod' ]; then | |||
composer install --prefer-dist --no-progress --no-suggest --no-interaction | |||
>&2 echo "Waiting for Postgres to be ready..." | |||
until bin/console doctrine:query:sql "SELECT 1" --quiet; do | |||
until pg_isready --timeout=0 --dbname="${DATABASE_URL}"; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we prefer using a Postgres database for performances reasons, this command definitively connects this entrypoint to the database system & driver. I don't find it very useful, especially if the final user wants to use a different system.
WDYT @dunglas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a better way. Thanks to the Sylius peeps who used it: https://github.com/Sylius/Sylius-Standard/blob/943125142f0267ac49903efeaab85e24de08424d/docker/php/docker-entrypoint.sh#L20-L23
Let's adopt the same method as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the same, but what is the purpose of using >/dev/null 2>&1
instead of --quiet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used this Doctrine command long ago, but I can't remember why we switched. pg_isready
looks more reliable anyway.
Even if we prefer using a Postgres database for performances reasons, this command definitively connects this entrypoint to the database system & driver. I don't find it very useful, especially if the final user wants to use a different system.
It's the same for the PDO driver and so on. The entrypoint is part of the infrastructure, to me it's ok and intended to couple it with the services we use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, please update api-platform/api-platform
first, and the demo after by copying it.
10cf5c2
to
8882512
Compare
We should be consistent with https://github.com/api-platform/api-platform/blob/master/api/docker/php/docker-entrypoint.sh#L25 closing this |
Following issue #31 and the comment #31 (comment)