development: Use native healthchecks in Docker compose#1257
development: Use native healthchecks in Docker compose#1257eulerbutcooler wants to merge 3 commits intometabrainz:masterfrom
Conversation
|
@MonkeyDo do lemme know if any changes are needed |
|
Hi @eulerbutcooler Thank you for opening this PR.... Please fill in the "AI usage" section in the PR template, it is there for a reason... |
I don't think I used AI in this one tho |
|
@eulerbutcooler If no AI was used in this submission, simply uncheck the AI usage box in the template. Furthermore, please ensure you use the new, mandatory PR template for your next contributions... You can refer to my PR #1248 to see how it should be properly filled out... : ) |
Ah alright. The PR still shows the old template. Will use the new one in the next PRs. Will update this one. Thanks for the heads up |
|
@eulerbutcooler For better understanding - metabrainz/listenbrainz-server#3610 |
ive updated the PR comment, is it fine now? |
|
@eulerbutcooler Nice !! Please update all your open PRs according to this template |
|
@eulerbutcooler Thanks for the PR, that's a much better way to do it, as you mentioned this was outdated. @faizanakhtar123 I appreciate the assist, but in this case I am at faults, not having changed the PR template for the BookBrainz repo. |
MonkeyDo
left a comment
MonkeyDo
left a comment
There was a problem hiding this comment.
Nice, good improvement thank you!
I tested locally, working fine.
You need to apply the same changes to the API docker files (docker-compose.api.yml and develop-api.sh)
As mentioned below, please also test setting up the repo from scratch and report your results
| volumes: | ||
| - postgres-data:/var/lib/postgresql/data | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "pg_isready -U bookbrainz -d bookbrainz"] |
There was a problem hiding this comment.
question: will this health check fail if you do not have the BB database set up (i.e. your very first setup run)?
If you haven't yet, it would be good to test a setup from scratch using this dockerfile to ensure we're not breaking anything.
There was a problem hiding this comment.
Hey! went through the postgres docs and some stackoverflow posts and no it wont fail if we don't have the BB database already set up. It only checks if the postgres server is responsive or not.
Considering this I've updated the query to psql -U bookbrainz -d bookbrainz SELECT 1 - this ensures the database exists.
And yes I've tested out locally setting up the repo from scratch using the dockerfile and had no issues.
I've added the requested changes in the upcoming commit.
There was a problem hiding this comment.
@MonkeyDo let me know if any other changes are needed. Thanks!
Let's not pour oil on the fire. Now that it has been updated, as a maintainer, I would answer yes: if someone did not fill in the new template, I would expect they used AI and didn't even bother to fill the PR description themselves. |
…eck method for postgres in docker-compose.yml and docker-compose.api.yml
MonkeyDo
left a comment
MonkeyDo
left a comment
There was a problem hiding this comment.
Thanks, a good improvement and one less dependency!
MonkeyDo
changed the title
| volumes: | ||
| - postgres-data:/var/lib/postgresql/data | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "psql -U $$POSTGRES_USER -d $$POSTGRES_DB -c 'SELECT 1'"] |
There was a problem hiding this comment.
I spoke too soon :)
I tested this with no database, and the healthcheck passed even without a database.
Not sure exactly how you tested this, but probably not thoroughly enough in any case.
I think it's because POSTGRES_DB isn't necessarily defined and the default value results in a valid healthcheck.
At this moment I don't think we want to force checking for the bookbrainz database, so we should revert this check to the previous simpler one using pg_isready, for both files.
There was a problem hiding this comment.
Hey @MonkeyDo I've reverted it back to pg_isready.
3 participants
Problem
Docker compose used external image (waisbrot/wait) to delay startup until postgres, elasticsearch and redis were ready. This is an outdated method of doing this.
Solution
I've updated the docker compose to use the docker native health check commands for all three postgres, elasticsearch and redis services.
Areas of Impact
This makes the docker compose file more maintainable.
TESTING
AI usage