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 healtcheck.sh scripts for jigasi, jicofo, jvb and prosody, add cu… #1845

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

denzs
Copy link
Contributor

@denzs denzs commented Jul 1, 2024

coming from jitsi/jigasi#539

We are using the container images with podman and are going to leverage the integrated healthcheck features..

This PR adds some basic healtcheck.sh scripts for jigasi, jicofo, jvb and prosody but does not add them as healthcheck in the Dockerfiles - because we are not using docker and i cant estimate if there are any sideeffects.

Anyhow i think having general healtcheck scripts available in the images would be a good thing :)

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Left some comments, PTAL! Also @aaronkvanmeerten WDYT?

prosody/Dockerfile Outdated Show resolved Hide resolved
jicofo/rootfs/healthcheck.sh Outdated Show resolved Hide resolved
jicofo/rootfs/healthcheck.sh Outdated Show resolved Hide resolved
@denzs
Copy link
Contributor Author

denzs commented Jul 1, 2024

@saghul i think i've considered every suggestion, thanks for the input! :)
Anyhow, Github still shows "Changes requested" - but i dont see any open discussions 🤷

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

LGTM! LEt's wait for @aaronkvanmeerten 's feedback.

Copy link
Member

@aaronkvanmeerten aaronkvanmeerten left a comment

Choose a reason for hiding this comment

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

LGTM, curious about the difference in hash for the mercurial source fetch, but not enough to block merging.

@@ -57,6 +57,7 @@ RUN wget -qO /etc/apt/trusted.gpg.d/prosody.gpg https://prosody.im/files/prosody
rm -rf /usr/share/lua/{5.1,5.2,5.3} && \
wget -qO /prosody-plugins/mod_auth_cyrus.lua https://hg.prosody.im/prosody-modules/raw-file/65438e4ba563/mod_auth_cyrus/mod_auth_cyrus.lua && \
wget -qO /prosody-plugins/sasl_cyrus.lua https://hg.prosody.im/prosody-modules/raw-file/65438e4ba563/mod_auth_cyrus/sasl_cyrus.lua && \
wget -qO /prosody-plugins/mod_http_health.lua https://hg.prosody.im/prosody-modules/raw-file/2b80188448d1/mod_http_health/mod_http_health.lua && \
Copy link
Member

Choose a reason for hiding this comment

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

This one has a different hash than the rest, not urgent but I'm curious if we can either have the same hash or update the others to a newer one? Doesn't have to be in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for merging! :)

Let me try to satisfy your curiosity.. ;)

2b80188448d1/5924 refers to the current tip (June 2024) of the hg repository.
65438e4ba563/4929 refers to a commit from Apr 18 2022.

mod_http_health was added in 5667:9bcd257dea4e (Oct 2023).

That is why 65438e4ba563 was not feasible for mod_http_health.

After diffing 65438e4ba563 and 2b80188448d1 i can confirm, there are no differences in mod_auth_cyrus..

So there is nothing to be said against using the current tip 2b80188448d1 for mod_auth_cyrus too.

Want a PR? :)

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case, I'd say there is no need!

@aaronkvanmeerten aaronkvanmeerten merged commit c65d2bd into jitsi:master Jul 2, 2024
1 check passed
@denzs denzs deleted the add_healthcheck_scripts branch July 3, 2024 05:54
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