You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks for maintaining this, this is fantastic, and a very helpful resource. I literally use this container every day. I do not mean to come in and try to change things, but I have some thoughts about some improvements.
I'd like to rewrite this such that odoo.conf is merged with available environment variables at startup. This is very easily achievable with envsubst. This lets users pick and choose to override odoo.conf settings with environment variables.
Once generated, the wait-for-psql.py should load the configuration file with configparser to wait for the databases specified in the Odoo configuration to be available.
I am okay if you do not want to merge these changes, but I will contribute my changes in this direction. It would be helpful if anyone has any criticisms of this or ideas.
Proposal
We should change the way this script works so that it merges the environment variables into the odoo.conf file. wait-for-psql.py should be changed to read the resulting file, and should load relevant data from the odoo.conf file. The entrypoint.sh file should be responsible for generating the final odoo.conf, should execute Odoo without hard-coding command-line arguments, and database passwords should not be loaded into logs or the terminal.
Consider the following default configuration file:
We can then update the entrypoint.sh file to read these variables. You do not need to read them in this file, but this is how we can specify defaults for the configuration items. I have specified the defaults from the current odoo.conf:
Users can uncomment or hard-code the value in the .conf file. This lets them pick what they want to come from the environment. By default, its just the database details.
You then need to change the wait-for-psql.py file to read those values instead of the args as it was doing before. You can read the resulting config file with the configparser library and get the database details directly from the odoo config. This way, if the user uploads their own odoo.conf they do not need to re-specify the details:
This version supports loading the configuration from the ODOO_RC environment variable, or will default to the /etc/odoo/odoo.conf
Finally, you can now configure basically everything from docker compose or ECS's in-browser environment variables, or you can specify the details from something like a secret manager or even aws s3.
Un-comment the configuration you want to come from the .env in the odoo.conf file, and you're good to go.
I am wrapping up these changes and can submit this as a PR, but it would be good to have some additional thoughts about this, and if you're open to the change.
As it stands these changes seem to be working fine:
Does anyone else have any thoughts about this or why it shouldn't be done?
The text was updated successfully, but these errors were encountered:
Overview
Thanks for maintaining this, this is fantastic, and a very helpful resource. I literally use this container every day. I do not mean to come in and try to change things, but I have some thoughts about some improvements.
I'd like to rewrite this such that
odoo.conf
is merged with available environment variables at startup. This is very easily achievable withenvsubst
. This lets users pick and choose to overrideodoo.conf
settings with environment variables.Once generated, the
wait-for-psql.py
should load the configuration file with configparser to wait for the databases specified in the Odoo configuration to be available.I am okay if you do not want to merge these changes, but I will contribute my changes in this direction. It would be helpful if anyone has any criticisms of this or ideas.
Proposal
We should change the way this script works so that it merges the environment variables into the odoo.conf file.
wait-for-psql.py
should be changed to read the resulting file, and should load relevant data from the odoo.conf file. Theentrypoint.sh
file should be responsible for generating the finalodoo.conf
, should execute Odoo without hard-coding command-line arguments, and database passwords should not be loaded into logs or the terminal.Consider the following default configuration file:
A user can hard-code a value, or pull the value from the environment.
For example:
will be a hard-coded value in the config and remain untouched by these changes, but:
will set the value from the environment variable.
This can be accomplished by adding the
gettext
library to Ubuntu, which containsenvsubst
. To do this, edit theDockerfile
:RUN apt-get update && \ DEBIAN_FRONTEND=noninteractive \ apt-get install -y --no-install-recommends \ ... gettext \ ... ca-certificates \ curl \ npm \
We can then update the
entrypoint.sh
file to read these variables. You do not need to read them in this file, but this is how we can specify defaults for the configuration items. I have specified the defaults from the currentodoo.conf
:This results in the following configuration file:
Users can uncomment or hard-code the value in the
.conf
file. This lets them pick what they want to come from the environment. By default, its just the database details.You then need to change the
wait-for-psql.py
file to read those values instead of the args as it was doing before. You can read the resulting config file with theconfigparser
library and get the database details directly from the odoo config. This way, if the user uploads their own odoo.conf they do not need to re-specify the details:This version supports loading the configuration from the ODOO_RC environment variable, or will default to the
/etc/odoo/odoo.conf
Finally, you can now configure basically everything from docker compose or ECS's in-browser environment variables, or you can specify the details from something like a secret manager or even aws s3.
Un-comment the configuration you want to come from the
.env
in theodoo.conf
file, and you're good to go.I am wrapping up these changes and can submit this as a PR, but it would be good to have some additional thoughts about this, and if you're open to the change.
As it stands these changes seem to be working fine:
Does anyone else have any thoughts about this or why it shouldn't be done?
The text was updated successfully, but these errors were encountered: