Containerize Order service: multi-stage Dockerfile, /health + /ready, env config#78
Containerize Order service: multi-stage Dockerfile, /health + /ready, env config#78devin-ai-integration[bot] wants to merge 2 commits into
Conversation
… env-based config, .dockerignore
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| app.MapHealthChecks("/health", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("live") | ||
| }); |
There was a problem hiding this comment.
🔍 Health endpoint path inconsistent with all other services in the repo
All other services in this repo (Identity, Customer, Product, Notification, ApiGateway) expose their health check at /healthz (see e.g. src/Services/Customer/Customer.API/Program.cs:23, src/Services/Identity/Identity.API/Program.cs:23). This PR changes Order to use /health and /ready instead. If there is any shared infrastructure (load balancer rules, Kubernetes ingress, monitoring dashboards) that probes /healthz across all services, the Order service will appear unhealthy. The Dockerfile HEALTHCHECK on line 40 correctly targets /health to match the new code, but the divergence from the fleet-wide convention is worth a deliberate decision.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The /health + /ready split was an explicit requirement for this task (separate liveness and readiness probes). Valid point about fleet consistency though — if there's shared infra probing /healthz, I can add /healthz back as an alias mapping to the liveness check so Order stays compatible while still exposing the new /health and /ready endpoints. Want me to add that alias here, or would you rather roll /health+/ready out across the other services separately?
| builder.Services.AddHealthChecks() | ||
| .AddCheck("self", () => HealthCheckResult.Healthy(), tags: new[] { "live" }) | ||
| .AddDbContextCheck<OrderDbContext>("orderdb", tags: new[] { "ready" }); |
There was a problem hiding this comment.
🔍 Container HEALTHCHECK only runs the always-healthy self check, never detects database failures
The Dockerfile HEALTHCHECK curls /health (src/Services/Order/Order.API/Dockerfile:40), which is mapped with Predicate = check => check.Tags.Contains("live") at src/Services/Order/Order.API/Program.cs:33. The only check tagged "live" is the self-check that unconditionally returns HealthCheckResult.Healthy() (line 18). This means the Docker-level health check will never detect a database outage. The DB check is only on /ready (tagged "ready"). This is a valid liveness-vs-readiness separation pattern for Kubernetes, but in a plain Docker or docker-compose deployment (which is what this repo uses — see src/docker-compose.yml), there is no orchestrator querying /ready, so database failures will go completely undetected by the container runtime.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional. The Docker HEALTHCHECK targets /health (liveness) on purpose: a container's health status should reflect whether the process itself is alive, not whether a downstream dependency is up. Tying the HEALTHCHECK to the DB (/ready) would cause the container to be marked unhealthy — and potentially restart-looped — during a transient Postgres outage, even though the app is fine and would recover on its own once the DB returns. /ready is still exposed for orchestrators/LBs that want to gate traffic on dependency health. If you'd prefer the compose deployment to also surface DB reachability at the container level (e.g. for depends_on: condition: service_healthy), I can point the HEALTHCHECK at /ready instead — let me know.
Summary
Containerizes the Order service (
src/Services/Order/Order.API) with a hardened image, dedicated liveness/readiness probes, and fully externalized configuration.sdk:10.0build →aspnet:10.0runtime): layer-cached restore,dotnet publish, and a slim final stage. Runs as the non-rootappuser (USER $APP_UID, uid 1654) and declares a containerHEALTHCHECKhitting/health.Program.cs:/health— liveness, taggedlive(trivialselfcheck, always up while the process runs)./ready— readiness, taggedready, backed byAddDbContextCheck<OrderDbContext>()so it returns503until Postgres is reachable and200once it is.Microsoft.Extensions.Diagnostics.HealthChecks.EntityFrameworkCore.)ConnectionStrings:DefaultConnection(Host=localhost;...;Password=postgres) fromappsettings.json. The connection string now comes solely from the environment (ConnectionStrings__DefaultConnection), matching howdocker-compose.ymlalready wires it. No hosts/credentials are baked into the image; the listen port is also env-driven (APP_PORT/ASPNETCORE_URLS)..dockerignoreadded at the build-context root (src/) to excludebin/,obj/,.git/, IDE files, and local.env/appsettings.*.local.jsonsecrets from the build context.Note: the previous
/healthzroute is replaced by/health+/ready.Verification
Built the image and ran the container:
docker build -f Services/Order/Order.API/Dockerfile -t order-service:test .→ success.GET /health→200 Healthy,GET /ready→503 Unhealthy.postgres:16-alpinecontainer on a shared network:GET /health→200,GET /ready→200 Healthy.docker inspectHEALTHCHECK status →healthy;docker exec ... id→uid=1654(app)(non-root).Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/1bb4867c148e483a853062ef04e8065d
Requested by: @mbatchelor81