Skip to content
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

Add DATABASE environment variable #537

Closed
wants to merge 3 commits into from
Closed

Conversation

codebykyle
Copy link

Per the discussion in this ticket:

#535

This pull request adds support for a specific database while maintaining the original functionality if a database name is not specified or the database argument is blank. This addresses the concern that specifying a database name would lock Odoo to the postgres table

Add a DATABASE=mydatabase environment variable to select a specific database

Tests and examples here:
#535 (comment)

Add a --database param which defaults to postgres
Add a database param which is removed if not set
@codebykyle
Copy link
Author

codebykyle commented Feb 10, 2025

Per the comment here:
#482 (comment)

This comment is correct, the db name can be a list of databases. Per the Odoo documentation, this list is a comma separated list:
https://www.odoo.com/documentation/18.0/developer/reference/cli.html#database

Patching the wait-for-psql.py file will let us split this out and wait for multiple databases. I will submit a fix.

@codebykyle
Copy link
Author

This now supports waiting for multiple databases.

DATABASE=mydatabase,test

will wait for mydatabase and test to be up before starting the Odoo container.

@codebykyle
Copy link
Author

codebykyle commented Feb 10, 2025

As a note, we can now support multiple databases, and the timeout is non-configurable.

You could imagine where you want, say, 20 databases, just the act of connecting to multiple may cause us to exceed the default timeout (which is set to 30, which should be plenty right now, and should not block this PR).

However, we would probably want this to be configurable in the event someone has a lot of databases. I will address this as a separate PR if we continue to use this as opposed to the changes in #496 PR

@codebykyle
Copy link
Author

Sorry I had an issue here, I will resubmit this PR along with some additional changes that will address this in a better way, this one should not be merged.

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

Successfully merging this pull request may close these issues.

1 participant