odoo / docker

Other
931 stars 1.51k forks source link

parameterize db_name #426

Open mohammed90 opened 1 year ago

mohammed90 commented 1 year ago

In case the database name is not postgres, the current image completely fails to run with the error:

Database connection failure: connection to server at "..." (ip-address), port ... failed: FATAL:  database "postgres" does not exist

This PR parameterizes the db name so the admin can spin up the image without having to hack-around this limitation.

Fixes #394

mschinis commented 1 year ago

can this please be merged?

swissbuechi commented 6 months ago

Thanks a lot. I hope it gets merged soon.

amh-mw commented 6 months ago

Per https://www.postgresql.org/docs/current/app-initdb.html,

Creating a database cluster consists of creating the directories in which the cluster data will live, generating the shared catalog tables (tables that belong to the whole cluster rather than to any particular database), and creating the postgres, template1, and template0 databases. The postgres database is a default database meant for use by users, utilities and third party applications. template1 and template0 are meant as source databases to be copied by later CREATE DATABASE commands. template0 should never be modified, but you can add objects to template1, which by default will be copied into databases created later. See Section 23.3 for more details.

Hence, the startup sequence for this container checks for the default database to be available, verifying the readiness of the postgres container, rather than verifying the existence of a particular Odoo database -- of which there may be more than one.

I already ran this down in https://github.com/odoo/docker/pull/482 and closed my own pull request because I determined this is a bad idea.

mohammed90 commented 6 months ago

Hence, the startup sequence for this container checks for the default database to be available, verifying the readiness of the postgres container, rather than verifying the existence of a particular Odoo database -- of which there may be more than one.

It can check for the database I command it to check, not the database the booting script decides to check. I'll share one set of credentials to the Odoo app, not 2, for the purpose of isolation and security. The current setup is a nightmare for multitenant hosting.

amh-mw commented 6 months ago

I understand your reasoning, but in this pull request, the additional --db_name argument to wait-for-psql.py is never passed from entrypoint.sh. When I got that far in my own experimentation, I noticed that entrypoint.sh sends the same arguments to both wait-for-psql.py and odoo, which causes non-multi-tenant setups to run Odoo out of the postgres database by default... which is less than ideal.

mohammed90 commented 6 months ago

which causes non-multi-tenant setups to run Odoo out of the postgres database by default... which is less than ideal.

It's doing that already without the PR, which is... Less than ideal.

lathama commented 3 months ago

@mohammed90 can you use https://docs.docker.com/build/building/multi-stage/ to accomplish your use case? If so can you close this PR?

mohammed90 commented 3 months ago

@mohammed90 can you use https://docs.docker.com/build/building/multi-stage/ to accomplish your use case? If so can you close this PR?

I don't understand how multi-stage build is supposed to help. Can you elaborate?

lathama commented 3 months ago

I guess it does not need to be multi stage

Dockerfile

FROM docker.io/odoo:17.0 as odoodbname
COPY myversion_wait-for-psql.py /usr/local/bin/wait-for-psql.py

Then build

docker build -t odoo:myversion

Then you have your version.

Note: quickly typed out, spelling and typos are free of charge. :)

mohammed90 commented 3 months ago

I guess it does not need to be multi stage

Dockerfile

FROM docker.io/odoo:17.0 as odoodbname
COPY myversion_wait-for-psql.py /usr/local/bin/wait-for-psql.py

Then build

docker build -t odoo:myversion

Then you have your version.

Note: quickly typed out, spelling and typos are free of charge. :)

I fail to see how that is a proper solution. It's a hack at best. Appreciate the help, but I'm aiming to provide a solution that resolves root concern, not bandaid it.