Repository navigation
Conversation
- Retire le template WeatherForecast (contrôleur, modèle, requête .http) - Ajoute MoissonDbContext dans Data/ et l'enregistre avec Npgsql - Lit ConnectionStrings:Moisson depuis la configuration (user-secrets en local, variables d'environnement sinon) ; l'API refuse de démarrer si la chaîne est absente - Fixe Microsoft.EntityFrameworkCore.Relational en 10.0.13 pour régler le conflit de versions d'EF Core dans Tests
- Ajoute le package HealthChecks.EntityFrameworkCore 10.0.13 - Expose /health : Healthy (200) si PostgreSQL répond, Unhealthy (503) sinon - Rend le message d'erreur de connexion autonome (commande user-secrets incluse)
- WebApplicationFactory pour démarrer l'API en mémoire - Testcontainers pour une base PostgreSQL 17 jetable - Vérifie Healthy (200) quand la base répond et Unhealthy (503) quand elle est injoignable
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| <PrivateAssets>all</PrivateAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="Microsoft.EntityFrameworkCore.Relational" Version="10.0.13" /> |
There was a problem hiding this comment.
Microsoft.EntityFrameworkCore.Relational 10.0.13 is already provided by Npgsql
| <PackageReference Include="Microsoft.EntityFrameworkCore.Relational" Version="10.0.13" /> | |
There was a problem hiding this comment.
Bien vu, merci ! Je l'avais ajouté pour régler un conflit de versions EF Core (10.0.4 vs 10.0.13) dans le projet Tests, mais depuis l'ajout de HealthChecks.EntityFrameworkCore 10.0.13 il est redondant.
| @@ -20,4 +34,6 @@ | |||
|
|
|||
| app.MapControllers(); | |||
|
|
|||
| app.MapHealthChecks("/health"); | |||
There was a problem hiding this comment.
MVC configuration is currently pointless, but we will most likely add controllers soon. We can keep them for now
| "La chaîne de connexion 'Moisson' est absente. Définissez ConnectionStrings__Moisson " + | ||
| "ou utilisez 'dotnet user-secrets'. "); |
There was a problem hiding this comment.
Remove space at the end of error message
| "La chaîne de connexion 'Moisson' est absente. Définissez ConnectionStrings__Moisson " + | |
| "ou utilisez 'dotnet user-secrets'. "); | |
| "La chaîne de connexion 'Moisson' est absente. Définissez ConnectionStrings__Moisson " + | |
| "ou utilisez 'dotnet user-secrets'."); |
- Déjà fournie en 10.0.13 via HealthChecks.EntityFrameworkCore - Retire un espace en trop dans le message d'erreur de connexion
lauriechammah
left a comment
There was a problem hiding this comment.
Overall good. For the future, make sure that your PR and commit messages are written in English.
Please fix the issues I have mentioned in my comments. Thank you! :)
|
|
||
| var builder = WebApplication.CreateBuilder(args); | ||
|
|
||
| var connectionString = builder.Configuration.GetConnectionString("Moisson") |
There was a problem hiding this comment.
Cloning the repo causes a crash on dotnet run: ConnectionStrings:Moisson isn't defined anywhere, even though compose.yaml already sets the dev credentials. Could we add the local dev connection string to appsettings.Development.json?
"ConnectionStrings": {
"Moisson": "Host=localhost;Port=5432;Database=moisson_dev;Username=moisson;Password=moisson"
}These are local-only Docker credentials, so committing them is fine. User-secrets/env vars can still override them.
|
|
||
|
|
||
| // Add services to the container. | ||
| builder.Services.AddDbContext<MoissonDbContext>(options => options.UseNpgsql(connectionString)); |
There was a problem hiding this comment.
Heads-up for the migrations issue: because the connection string is read and validated at startup (line 7), dotnet ef migrations add will throw on any machine/CI without the secret, since EF runs Program to build the DbContext. Fixing the dev config (comment above) mostly solves it. Otherwise, add an IDesignTimeDbContextFactory.
|
|
||
| app.MapControllers(); | ||
|
|
||
| app.MapHealthChecks("/health"); |
There was a problem hiding this comment.
/health sits behind UseHttpsRedirection(), so an HTTP health probe (Docker, Kubernetes, load balancer) gets a 307 instead of the status and may mark the service as down. Not a problem today, but worth keeping in mind when we deploy (e.g. skip the redirect for /health).
|
|
||
| public class HealthEndpointTests : IAsyncLifetime | ||
| { | ||
| private readonly PostgreSqlContainer _postgres = new PostgreSqlBuilder("postgres:17").Build(); |
There was a problem hiding this comment.
xUnit creates a new instance of the class per test, so this starts a new Postgres container for each test (including the "unreachable" one, which doesn't use it). Moving the container into a shared IClassFixture<PostgresFixture> would start it only once and speed up the suite.
|
|
||
| namespace MoissonMontreal.Tests.Api; | ||
|
|
||
| public class HealthEndpointTests : IAsyncLifetime |
There was a problem hiding this comment.
These tests require Docker to be running, otherwise dotnet test fails with DockerUnavailableException. Could we tag them (e.g. [Trait("Category", "Integration")]) so they can be filtered, and mention the Docker requirement in the README?
| public async Task Health_RetourneUnhealthy_QuandLaBaseEstInjoignable() | ||
| { | ||
| // Rien n'écoute sur le port 1 : la connexion échoue immédiatement. | ||
| await using var factory = CreerFactory("Host=localhost;Port=1;Database=inexistante;Username=test;Timeout=2"); |
There was a problem hiding this comment.
Minor: localhost can resolve to both ::1 and 127.0.0.1, and some CI firewalls drop packets instead of refusing them, so this can wait for the timeout twice instead of failing immediately. Using Host=127.0.0.1 makes it more predictable.
anis022
left a comment
There was a problem hiding this comment.
@lauriechammah requested changes cover pretty much everything that needs to be addressed. Other than that, looks good to me. Great work.
Closes #11
Overview
Base de l'API centrale et connexion à PostgreSQL.
MoissonDbContext(vide pour l'instant, le schéma viendra avec son issue)/healthqui vérifie la connexion à la base (Healthy200 /Unhealthy503)/health(WebApplicationFactory + Testcontainers)Tests automatisés
dotnet test` : 3/3 réussis (Docker requis pour Testcontainers)
tests/MoissonMontreal.Tests/Api/HealthEndpointTests.cs
Checklist
| Critère | Test | Résultat |
| The API runs locally |
dotnet run --project src/MoissonMontreal.Api| ✅Now listening on: http://localhost:5297|| Health endpoint reports DB reachability |
curl -i localhost:5297/healthavec la base allumée | ✅200 OK+Healthy|| |
docker compose stop dbpuiscurl -i localhost:5297/health| ✅503 Service Unavailable+Unhealthy|| No credentials committed |
git grep -n "Password=" -- ':!compose.yaml'| ✅ aucun résultat |