-
Notifications
You must be signed in to change notification settings - Fork 724
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 docker healthchecks to the docker images #1027
Conversation
Signed-off-by: Sanoj <[email protected]> Signed-off-by: Sanoj <[email protected]>
Signed-off-by: Sanoj <[email protected]> Signed-off-by: Sanoj <[email protected]>
Signed-off-by: Sanoj <[email protected]>
@Z0ey2022 Thanks for the PR! I'll have a look soon! |
Short question: why do you/we need these health checks? |
I will wait for #1044 to be merged, so I can directly add a Healthcheck for elastic search |
Signed-off-by: Zoey <[email protected]>
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.
Thanks! LGTM in general!
Just a few questions/regards.
Signed-off-by: Zoey <[email protected]>
Containers/fulltextsearch/Dockerfile
Outdated
@@ -2,3 +2,5 @@ | |||
FROM elasticsearch:7.17.6 | |||
|
|||
RUN elasticsearch-plugin install --batch ingest-attachment | |||
|
|||
HEALTHCHECK CMD curl -skfI localhost:9200 || exit 1 |
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.
we don't need this as the base image already includes the healthcheck. See https://github.com/elastic/elasticsearch/blob/8b3293ddeb008459bf795cb4082beab3d654cd88/distribution/docker/src/docker/Dockerfile#L295
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.
done
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.
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.
Strange! Seems like you are right.
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.
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.
Can you add this back?
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.
Signed-off-by: Zoey <[email protected]>
Signed-off-by: Zoey <[email protected]>
LGTM now! Please rebase on top of master and squash your changes. Afterwards I'll merge! |
Sorry, but I don't get them rebased, should I create a new PR with these changes? |
Fine by me if that is easier for you 👍 |
Added Docker Health checks to some containers.
clamav container already includes this.
And for the watchtower & borgbackup container I don't know how this could be done, and I think, because they only run for a short time, it wouldn't make much sense to add this check to them.
In my test (I connected using bash, for example:
docker exec -it nextcloud-aio-collabora bash
, the executed the health check commands and then checked withecho $?
) all checks reported Exit Code 0.