-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: use mariadb client for healthchecks #221
base: main
Are you sure you want to change the base?
Conversation
Instead of grepping for an existing daemon, we now run a query similar to how `mysqladmin` does it (connects to server/db and runs `select 1`).
Findings:
|
CHECK="mariadb" | ||
|
||
# prefer root if available | ||
if [ -n "${MYSQL_ROOT_PASSWORD}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ENV is not reliable since it can be changed right after the first warm up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my first point/todo and the first comment. MariaDB health check ignores auth and mysql ignores healthchecks altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we want is a sane default, if people do changes they should probably override the healthcheck as well. I can see this being very common in any kind of setup where you orchestrate a docker compose setup with many actors.
CHECK="mariadb" | ||
|
||
# prefer root if available | ||
if [ -n "${MYSQL_ROOT_PASSWORD}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth providing ENVs for the check, and if they are all empty then it does nothing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are all empty it would try to connect without credentials. This is what mariadb does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea only if you can have separate variables, this makes this image deviate from the original mariadb one a bit too much for me
This is what the original does: $ mariadb -h localhost --protocol tcp -e 'select 1' If you set up this repo using the PR and remove your environment variables you'd see the same result. Edit: if you check the original issue about adding this functionality to MariaDB; they are talking about the same thing / what I am trying to do as default: MariaDB/mariadb-docker#94 The cleanest approach to me from a production standpoint is to initialize your container, no longer pass credentials in your compose, then mount a config that contains authentication in a I would add it to the documentation / best practices. That would be a great way to unit test this as well. What do you think? |
Instead of grepping for an existing daemon, we now run a query similar to how
mysqladmin
does it (connects to server/db and runsselect 1
).Todo
docker inspect --format "{{json .State.Health }}"
- I could just lean on similar query / logic)