Add HEALTHCHECK to APE Dockerfile#368
Conversation
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review: Add HEALTHCHECK to APE Dockerfile
Looks correct overall. The healthcheck targets port 7890 and /health, both matching the APE manifest (manifest.yaml port: 7890, health: /health) and EXPOSE 7890. The Python urllib approach is appropriate since the slim image has no curl/wget.
Minor notes
-
|| exit 1is redundant (line 15 in the diff): SinceHEALTHCHECK CMDuses shell form, Docker already uses the command's exit code. Ifurllib.request.urlopenfails, Python exits non-zero on its own. The|| exit 1is harmless but unnecessary — fine to keep for readability, just noting it. -
No env var expansion needed here: Unlike token-spy (which uses
UVICORN_PORT), APE hardcodes port 7890 in bothmain.pyand the Dockerfile, so the hardcoded URL is correct. Shell form vs exec form is not a concern for this specific healthcheck.
Overlap with PR #304 and #372
This PR directly conflicts with PR #304, which adds the same APE healthcheck (identical approach, but with --timeout=10s instead of 5s here) plus a token-spy healthcheck. PR #372 also adds the token-spy healthcheck separately.
All three PRs touch the same file (dream-server/extensions/services/ape/Dockerfile) with nearly identical changes. Merging any two will create a git conflict on the others. The maintainers should decide which to land first — #304 covers both services in one PR, while #368 and #372 split them. Either path works, but the timeout difference (5s here vs 10s in #304) should be reconciled.
Verdict: The change itself is clean and correct. No blocking issues — just coordinate merge order with #304/#372.
Summary
Adds Docker HEALTHCHECK directive to APE service for consistency with other services.
Changes
/healthendpoint using Python urllibConsistency
All other Python services in the stack have healthchecks:
APE was the only Python service missing this directive.