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 healthcheck #22

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add healthcheck #22

wants to merge 2 commits into from

Conversation

NFhook
Copy link

@NFhook NFhook commented Mar 7, 2024

No description provided.

@razvancrainea
Copy link
Member

Thank you for the contribution! The idea is great, the only problem is that the existence of opensips-cli depends on the OPENSIPS_CLI argument - which defaults to false. Therefore, the healthcheck should only be activated if OPENSIPS_CLI is true - can you please amend this accordingly?

@@ -22,7 +22,7 @@ RUN apt-get -y update -qq && \
apt-get -y install \
opensips${OPENSIPS_VERSION_MINOR:+=$OPENSIPS_VERSION.$OPENSIPS_VERSION_MINOR-$OPENSIPS_VERSION_REVISION}

ARG OPENSIPS_CLI=false
ARG OPENSIPS_CLI=true
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately this is not the correct fix, as one might compile it with OPENSIPS_CLI=false - for example this is how we build the sipssert image.
The correct fix would be to exit 1 if OPENSIPS_CLI is false, or skip the healthcheck altoghether - not sure that is possible though

Copy link
Author

Choose a reason for hiding this comment

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

You are correct. If OPENSIPS_CLI is false, and if the opensips-cli is not installed, I don't know of any other way to perform healthcheck. Please forgive me, my English is not very good.

Copy link
Member

Choose a reason for hiding this comment

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

Please keep the PR open, as soon as I have some time, I will try to adapt it myself :).
Thanks for your contribution!

@NFhook NFhook closed this Mar 15, 2024
@razvancrainea razvancrainea reopened this Mar 15, 2024
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.

2 participants