Add CI workflow (build, unit tests, integration tests)#5
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a GitHub Actions CI workflow for the .NET ToyDb project. The workflow builds the solution, runs unit tests, orchestrates Docker Compose services, waits for replica readiness, executes integration tests, and ensures cleanup across all completion states. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
39-50: Consider adding diagnostic output on health check failure.When the health check fails, it would be helpful to see Docker container logs to debug the issue.
🔍 Suggested improvement to add diagnostics on failure
done echo "Routing did not become ready in time" + docker compose -f docker-compose.yml -f docker-compose.override.yml logs exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 39 - 50, The CI health-check step ("Wait for routing to be ready") should emit diagnostic logs when the loop times out: after the final failed attempt, run and print container logs for the relevant services (e.g., routing and gateway) and/or run docker-compose logs with recent lines so reviewers can see failures, then exit non‑zero; update the step to capture and echo those diagnostics when hitting the "Routing did not become ready in time" path and include the health endpoint (/diagnostics/health) context in the log output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 24-25: The CI step named "Generate and trust HTTPS certificate"
runs dotnet dev-certs https --trust -ep ./certs/aspnetapp.pfx -p password which
can silently fail on Ubuntu runners; update this step to remove the --trust flag
(use dotnet dev-certs https -ep ./certs/aspnetapp.pfx -p password) or add an OS
guard so --trust runs only on macOS/Windows, ensuring the pipeline either only
exports the .pfx for Docker or conditionally attempts to trust the cert based on
runner OS.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 39-50: The CI health-check step ("Wait for routing to be ready")
should emit diagnostic logs when the loop times out: after the final failed
attempt, run and print container logs for the relevant services (e.g., routing
and gateway) and/or run docker-compose logs with recent lines so reviewers can
see failures, then exit non‑zero; update the step to capture and echo those
diagnostics when hitting the "Routing did not become ready in time" path and
include the health endpoint (/diagnostics/health) context in the log output.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ToyDbIntegrationTests/Tests/PartitioningTests.cs`:
- Around line 21-28: The ReplicaClient instances created in the
PartitioningTests constructor (_p1r1Client, _p1r2Client, _p2r1Client,
_p2r2Client) are missing the certificate validation skip flag; update their
construction to pass IntegrationTestConfig.SkipCertificateValidation (the same
way RoutingClient is constructed) so the replicas use the same
skip-certificate-validation behavior as RoutingClient.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
67-68: Consider extracting the replica count to a variable.The hardcoded
4creates implicit coupling with the Docker Compose configuration. If the number of replicas changes, this workflow will need a manual update.♻️ Extract to workflow-level env variable
jobs: build-and-test: runs-on: ubuntu-latest + env: + EXPECTED_REPLICAS: 4 steps:Then reference it:
- if [ "$serving" -ge 4 ]; then - echo "All 4 replicas are Serving" + if [ "$serving" -ge $EXPECTED_REPLICAS ]; then + echo "All $EXPECTED_REPLICAS replicas are Serving"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 67 - 68, Extract the hardcoded replica count into a workflow-level environment variable (e.g., REPLICA_COUNT) and reference that variable in the shell check instead of the literal 4; update the condition that currently reads if [ "$serving" -ge 4 ]; then to compare against the env variable (e.g., "$REPLICA_COUNT") and ensure the variable is exported or available to the job so other steps that rely on the replica count use the same value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 67-68: Extract the hardcoded replica count into a workflow-level
environment variable (e.g., REPLICA_COUNT) and reference that variable in the
shell check instead of the literal 4; update the condition that currently reads
if [ "$serving" -ge 4 ]; then to compare against the env variable (e.g.,
"$REPLICA_COUNT") and ensure the variable is exported or available to the job so
other steps that rely on the replica count use the same value.
d3e54b2 to
ee795d5
Compare
ee795d5 to
e71ffe6
Compare
Adds GitHub Actions workflow that:
Made with Cursor
Summary by CodeRabbit