Skip to content

Containerize Notification service (hardened image + health/ready probes)#83

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1783565718-containerize-notification
Open

Containerize Notification service (hardened image + health/ready probes)#83
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1783565718-containerize-notification

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Containerizes the Notification service: builds into a hardened image, exposes liveness/readiness probes, takes all config from the environment, and fixes the known build-breaking project references so the image can actually build. Scoped to Notification only.

Build-breaking fix (required for the image to build). Notification.API.csproj referenced Shared projects one directory too shallow, resolving to a non-existent Services/Shared:

- ..\..\Shared\Shared.Contracts\Shared.Contracts.csproj
- ..\..\Shared\Shared.Infrastructure\Shared.Infrastructure.csproj
+ ..\..\..\Shared\Shared.Contracts\Shared.Contracts.csproj
+ ..\..\..\Shared\Shared.Infrastructure\Shared.Infrastructure.csproj

(from src/Services/Notification/Notification.API you need 3 .. to reach src/Shared). No other services' references were touched.

Multi-stage Dockerfile (sdk:10.0 build → aspnet:10.0 runtime): per-.csproj COPY for layer-cached restore, curl installed in the runtime stage (not present in the aspnet image, needed by HEALTHCHECK), USER $APP_UID (non-root), hardcoded internal port 5005 (ASPNETCORE_URLS=http://+:5005; remap via -p), and HEALTHCHECK probing /health (liveness, not readiness — so transient DB outages don't restart the container).

/health + /ready (+ /healthz alias) in Program.cs:

builder.Services.AddHealthChecks()
    .AddDbContextCheck<NotificationDbContext>("database", tags: new[] { "ready" });
...
app.MapHealthChecks("/health",  new HealthCheckOptions { Predicate = _ => false });          // liveness
app.MapHealthChecks("/healthz", new HealthCheckOptions { Predicate = _ => false });          // liveness alias (fleet convention)
app.MapHealthChecks("/ready",   new HealthCheckOptions { Predicate = c => c.Tags.Contains("ready") }); // DB reachable

Added the Microsoft.Extensions.Diagnostics.HealthChecks.EntityFrameworkCore package for AddDbContextCheck.

Startup DB init made non-fatal. The existing db.Database.EnsureCreated() at startup is now wrapped in try/catch so liveness does not depend on the DB being reachable at boot (readiness reflects DB state instead). Without this the process crashes at startup when no DB is present, so /health could never return 200 without a DB.

Externalized config. Removed the hardcoded ConnectionStrings:DefaultConnection (Host=localhost;...;Password=postgres) from appsettings.json; it now comes only from ConnectionStrings__DefaultConnection, which compose already supplies.

.dockerignore added at the build-context root (src/) excluding build artifacts and local secrets.

Compose: the notification-service block already existed (port 5005, depends_on postgres+rabbitmq, connection string via env) — verified, no edit needed. docker compose config validates.

Verification (real Docker, before this PR)

  • dotnet build ... -c Release — succeeds, 0 warnings/errors.
  • Image builds from src/.
  • No DB: GET /health200 Healthy, GET /healthz200, GET /ready503 Unhealthy.
  • With a real postgres: GET /ready200 Healthy (notificationdb auto-created on startup).
  • docker inspect health status → healthy; docker exec ... iduid=1654(app) (non-root).
  • Test containers/network cleaned up afterward.

Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/a5e67a775bea4d8a9a34de7f7bd3fdbd
Requested by: @mbatchelor81


Open in Devin Review

@mbatchelor81 mbatchelor81 self-assigned this Jul 9, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

Open in Devin Review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 Default connection string removed only for Notification service, inconsistent with other services

The ConnectionStrings.DefaultConnection was removed from appsettings.json, but all other services (Order, Customer, Identity, Product) still have their connection strings in their respective appsettings.json files (e.g. src/Services/Order/Order.API/appsettings.json:9). In Docker, this works because docker-compose.yml provides the connection string via environment variable. For local development without Docker, GetConnectionString("DefaultConnection") returns null, which is passed to UseNpgsql(). The EnsureCreated() failure is caught by the new try/catch, and the /ready health check will report 503 — so the app starts but is non-functional for DB operations. This may be intentional (avoiding credentials in config), but the inconsistency with other services and lack of an appsettings.Development.json fallback is worth confirming.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Intentional — the goal here is externalizing config so no credentials are baked into the image; the connection string comes from ConnectionStrings__DefaultConnection, which compose supplies. The startup EnsureCreated() is wrapped in try/catch so liveness (/health) never depends on the DB, and /ready correctly reports 503 when the DB is unreachable (e.g. local run without Docker/env var). The inconsistency with the other services' appsettings.json is expected: this PR is scoped to Notification, and those services will be aligned separately.

Comment on lines +10 to +11
<ProjectReference Include="..\..\..\Shared\Shared.Contracts\Shared.Contracts.csproj" />
<ProjectReference Include="..\..\..\Shared\Shared.Infrastructure\Shared.Infrastructure.csproj" />

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 Shared project reference path fix is correct but other services still have the broken path

The change from ..\..\Shared\... to ..\..\..\Shared\... is a genuine fix. From src/Services/Notification/Notification.API/, the old path ../../Shared resolves to src/Services/Shared/ which does not exist — the actual location is src/Shared/. The new three-level-up path correctly resolves to src/Shared/. However, all other services (Order, Customer, Product, Identity) still use the two-level ..\..\Shared\... path in their .csproj files (e.g. src/Services/Order/Order.API/Order.API.csproj:10-11). Those references are equally broken. It's possible the solution file or a Directory.Build.props was masking this, but the paths are objectively wrong for both local and Docker builds.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, and intentional. This PR is deliberately scoped to containerizing Notification only, so I fixed only Notification.API.csproj — that fix is required because the Notification image can't build otherwise. The other services (Order, Customer, Product, Identity) have the same broken ..\..\Shared\... path but were already containerized in prior PRs and building, so I left them untouched to keep this PR scoped. Fixing them belongs in a separate change.

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.

1 participant