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

fix(widget): add docker container healthcheck for widget web #7953

Draft
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

zettlrobert
Copy link

What changed? Why was the change needed?

The community edition currently was not starting based on removed healthchecks and container dependencies.

Screenshots

image

Details

I added a simple node script to check if the web server is up and running that should set an example on how healthchecks could be integrated after removing the curl dependency has been removed from the base image.

Special notes for your reviewer

Ensure that the environment is configured correctly, all ports and variables are set in the .env file.

For testing purposes, build the novu-web docker image locally, and exchange the cloud image in the community-compose file for the locally available one.

If you want a failing healthcheck you could pass a port that is NOT running in the container to the healthcheck.

# Go to web directory
cd apps/web

# Build docker image
pnpm docker:build

In the docker compose-compose.yaml file replace (just local testing)

    # image: 'ghcr.io/novuhq/novu/web:2.1.1'
    image: novu-web

Spin up the local compose stack

Copy link

netlify bot commented Mar 18, 2025

👷 Deploy request for dashboard-v2-novu-staging pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2f10564

@zettlrobert zettlrobert changed the title bugfix/docker community web healthcheck fix: docker community container healthcheck Mar 18, 2025
@zettlrobert zettlrobert changed the title fix: docker community container healthcheck fix(docker/community): add container healthcheck Mar 18, 2025
@zettlrobert zettlrobert changed the title fix(docker/community): add container healthcheck fix(widget): add docker container healthcheck for widget web Mar 18, 2025
@jainpawan21 jainpawan21 requested a review from merrcury March 21, 2025 05:47
@Aaron-Ritter
Copy link
Contributor

@zettlrobert do we need the same check for all the different containers and not only web? Since I believe the removal of all checks in the pull request #7929 is requiring further consideration. Can we reintroduce this for all the containers?

@zettlrobert
Copy link
Author

zettlrobert commented Mar 25, 2025

@zettlrobert do we need the same check for all the different containers and not only web? Since I believe the removal of all checks in the pull request #7929 is requiring further consideration. Can we reintroduce this for all the containers?

@Aaron-Ritter
Yes, it is needed and I can introduce it.
I just wanted some feedback if this approach is okay with the maintainers, as I am quite unfamiliar with the code base and their standards.

if a container is a dependency for an other container it should not be
removed.

according to the documentation the compose argument none could be
applied: https://docs.docker.com/reference/dockerfile/#healthcheck
if a container is a dependency for an other container it should not be
removed.

according to the documentation the compose argument none could be
applied: https://docs.docker.com/reference/dockerfile/#healthcheck
@zettlrobert zettlrobert force-pushed the bugfix/docker-community-web-healthcheck branch from 2f10564 to 28b0856 Compare March 27, 2025 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants