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

Improve PSQL wait script #496

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

thatnerdjosh
Copy link

@thatnerdjosh thatnerdjosh commented Mar 31, 2024

Follow up to #292

  • Fix cleanup of connections while testing
  • Migrate to pg_isready

@thatnerdjosh thatnerdjosh changed the title Improve PSQL wait script [WIP] - Improve PSQL wait script Mar 31, 2024
Copy link

@lathama lathama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting limits of the docker build process and best practices.

@thatnerdjosh thatnerdjosh force-pushed the feature/improve_wait_for_psql branch 4 times, most recently from f00f001 to 4282175 Compare March 31, 2024 22:23
@thatnerdjosh
Copy link
Author

@lathama - apologies for the back and forth - was struggling with the symlinks a bit.

This is ready for review :)

@thatnerdjosh thatnerdjosh changed the title [WIP] - Improve PSQL wait script Improve PSQL wait script Mar 31, 2024
@thatnerdjosh thatnerdjosh force-pushed the feature/improve_wait_for_psql branch 4 times, most recently from 4035fbe to c446a5b Compare March 31, 2024 22:29
* support: Move to shared directory to avoid code drift.
* feat: Make DB name configurable
@thatnerdjosh thatnerdjosh force-pushed the feature/improve_wait_for_psql branch 3 times, most recently from fb45459 to 065aadd Compare March 31, 2024 22:42
@thatnerdjosh
Copy link
Author

@lathama - I don't think it's really ideal to maintain copy and paste of all those scripts, but I struggled to find a solution for that. I've ported all of the changes from the previous PR to all the new images

15.0/entrypoint.sh Outdated Show resolved Hide resolved
@lathama
Copy link

lathama commented Apr 1, 2024

The installed package postgresql-client includes a tool called pg_isready which could be used to remove a ton of complexity. I had the cycles to take a look into this. The pg_isready could be added to the entrypoint.sh and the extra Python file could be removed.

$ pg_isready --help
pg_isready issues a connection check to a PostgreSQL database.

Usage:
  pg_isready [OPTION]...

Options:
  -d, --dbname=DBNAME      database name
  -q, --quiet              run quietly
  -V, --version            output version information, then exit
  -?, --help               show this help, then exit

Connection options:
  -h, --host=HOSTNAME      database server host or socket directory
  -p, --port=PORT          database server port
  -t, --timeout=SECS       seconds to wait when attempting connection, 0 disables (default: 3)
  -U, --username=USERNAME  user name to connect as

Report bugs to <[email protected]>.
PostgreSQL home page: <https://www.postgresql.org/>

@thatnerdjosh
Copy link
Author

Amazing - thank you @lathama - I'll migrate to this shortly

@thatnerdjosh
Copy link
Author

@lathama - can you confirm if the below items are ok:

  1. This returns 0 even in the case of failed authentication
  2. There is no option to pass a DB password

This is fine if all we need to do is verify the DB is ready to accept connections, but seems less so if we want to validate a connection is actually established.

Please advise - thanks!

@thatnerdjosh
Copy link
Author

Pending your feedback, I've completed the migration.

@lathama
Copy link

lathama commented Apr 1, 2024

Looking now

Copy link

@amh-mw amh-mw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #358, #394, #426 and #482.

Use cases:

  1. permissive single-tenant: wait for postgres database, odoo startup creates odoo database, --db_name set, --no-database-list
  2. permissive multi-tenant: wait for postgres database, use database manager to create database, --db_name unset
  3. restrictive multi-tenant (multiple odoo servers, single database server): named database already created, use that database, --db_name set, --no-database-list

15.0/entrypoint.sh Outdated Show resolved Hide resolved
15.0/entrypoint.sh Outdated Show resolved Hide resolved
15.0/entrypoint.sh Outdated Show resolved Hide resolved
15.0/entrypoint.sh Show resolved Hide resolved
@thatnerdjosh
Copy link
Author

@amh-mw - I've removed the DB_NAME override from this PR since the bulk of the changes was adding the pg_isready support. Once that is reviewed, I'll start working on the DB_NAME override :)

@thatnerdjosh thatnerdjosh force-pushed the feature/improve_wait_for_psql branch from 06268bf to cf77605 Compare April 7, 2024 23:36
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.

3 participants