Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated /health/ endpoint for the local development stack, updating the Docker Compose healthcheck and corresponding tests to use this new path. Feedback suggests wrapping the database query in the health view with exception handling to prevent potential information disclosure during database failures, and using the Django test client instead of RequestFactory to verify the endpoint through the full middleware stack.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Docker web health check currently requests the homepage. With a full catalog dump, that page can take longer than the health check's three-second request timeout, so Compose can report an unhealthy web service even though Django and MySQL are working.
I added a small
/health/endpoint that performs a singleSELECT 1and returns{"status": "ok"}, then pointed the Compose health check at that endpoint. I kept the existing three-second threshold so this checks actual application and database readiness without hiding slow requests behind a longer timeout. Database failures are logged and return a small JSON 503 response rather than Django's exception page.While validating the first CI run, I also found that the existing
mysqladmin pingprobe returned exit code 0 even when authentication failed. That could release the migration container before a new MySQL instance finished initialization. I changed the database probe to run an authenticatedSELECT 1against the configured database and added the exact command to the Compose contract test.I added coverage for the web route, response, direct one-query behavior, full middleware request, database-failure response, and both Compose health-check contracts.
Verification
I tested this against my local production-sized dump:
docker compose up -d --build --waitcompleted with the web service healthy/health/: 200 in 0.003-0.008 seconds across three requests./bin/dev doctor: Python 3.13.15, Django 5.2.17, MySQL 8.0.46; no system-check issuesI also rehearsed CI's clean-install path with a separate Compose project and fresh empty volume. The authenticated database probe waited for initialization, migrations completed, and the web service became healthy. Invalid database credentials returned exit code 1 instead of a false healthy result.