Repository navigation
ci: give the postgres service only to the suite that uses it - #593
Conversation
A service container is pulled before the job starts, so a registry hiccup fails the job outright, before any test runs. Two CI runs died this way in one day — one on E2E, one on Integration — each on the same anonymous pull of postgres:18-alpine timing out against registry-1.docker.io. Only the unit suite ever connects: GOMODEL_TEST_POSTGRES_URL is read solely by internal/storage/sqlx/sqlxtest, which the tests/* suites do not import, and the integration suite brings up its own containers in TestMain. So three of the four matrix legs were pulling a database they never opened, putting them at that risk for no coverage. Unit tests now run as their own job with the service; the rest share a matrix with none. Remaining pulls go through the AWS ECR Public mirror of Docker Hub's official library instead of Docker Hub itself, whose anonymous endpoint is the part that keeps timing out. The manifests are byte-identical to the Docker Hub ones, so this changes where the images come from, not what they are. Job names are unchanged, so required status checks keep matching. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CI workflow now runs PostgreSQL-backed unit tests in a dedicated job, while the shared matrix covers E2E, integration, and contract suites. Integration tests pre-pull AWS ECR Public mirror images with retries before starting PostgreSQL and MongoDB containers. ChangesCI test infrastructure
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant PostgreSQL
participant GoTests
participant Codecov
GitHubActions->>PostgreSQL: Start service container
GitHubActions->>GoTests: Run make test-race with PostgreSQL URL
GoTests->>PostgreSQL: Execute PostgreSQL-backed subtests
GitHubActions->>Codecov: Upload coverage
sequenceDiagram
participant TestMain
participant DockerPull
participant Docker
participant DatabaseContainers
TestMain->>DockerPull: Pull PostgreSQL and MongoDB images
DockerPull->>Docker: Retry failed pulls with backoff
DockerPull-->>TestMain: Return pull result
TestMain->>DatabaseContainers: Start PostgreSQL and MongoDB containers
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 88-93: Update the “Upload coverage” step to include an always-run
condition using always(), ensuring it executes even when the preceding make
test-race step fails while retaining its existing non-fatal upload behavior.
- Line 89: Pin every GitHub Action reference in .github/workflows/test.yml at
lines 89, 113, and 116 to its full reviewed immutable commit SHA, replacing the
mutable version tags while retaining an adjacent # vX.Y.Z version comment for
maintainability.
- Line 60: Replace the mutable Postgres and Mongo image tags with their reviewed
SHA256 digests in .github/workflows/test.yml at lines 60 and
tests/integration/main_test.go at lines 47-48, covering postgres:18-alpine,
postgres:16-alpine, and mongo:7 while preserving the existing image selections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1b6cecc-8c69-4e87-aa18-ebb7b5337207
📒 Files selected for processing (2)
.github/workflows/test.ymltests/integration/main_test.go
| # Mirrors Docker Hub's official image. Anonymous pulls of | ||
| # registry-1.docker.io intermittently time out on GitHub runners, which | ||
| # fails the job before a single test runs. | ||
| image: public.ecr.aws/docker/library/postgres:18-alpine |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files:"
git ls-files | rg '(^|/)(test\.yml|main_test\.go)$' || true
echo
echo "Relevant workflow/test references:"
for f in .github/workflows/test.yml tests/integration/main_test.go; do
if [ -f "$f" ]; then
echo "=== $f ==="
wc -l "$f"
rg -n 'image:|postgresImage|mangoImage|mongoImage|postgres|mongo|action@|uses:' "$f" -C 2 || true
fi
done
echo
echo "Show exact lines if available:"
if [ -f .github/workflows/test.yml ]; then
sed -n '55,65p' .github/workflows/test.yml
fi
if [ -f tests/integration/main_test.go ]; then
sed -n '40,55p' tests/integration/main_test.go
fiRepository: ENTERPILOT/GoModel
Length of output: 8431
Pin mirrored container images by digest.
The ECR Public references here are still mutable tags, so public.ecr.aws/docker/library/postgres:18-alpine, public.ecr.aws/docker/library/postgres:16-alpine, and public.ecr.aws/docker/library/mongo:7 can be retargeted. Replace each with its reviewed @sha256:... digest in .github/workflows/test.yml and tests/integration/main_test.go.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 60-60: unpinned image references (unpinned-images): container image is not pinned to a SHA256 hash
(unpinned-images)
📍 Affects 2 files
.github/workflows/test.yml#L60-L60(this comment)tests/integration/main_test.go#L47-L48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml at line 60, Replace the mutable Postgres and
Mongo image tags with their reviewed SHA256 digests in
.github/workflows/test.yml at lines 60 and tests/integration/main_test.go at
lines 47-48, covering postgres:18-alpine, postgres:16-alpine, and mongo:7 while
preserving the existing image selections.
Source: Linters/SAST tools
| - name: Upload coverage | ||
| if: matrix.coverage | ||
| uses: codecov/codecov-action@v7 | ||
| with: | ||
| files: ./coverage.out | ||
| fail_ci_if_error: false | ||
| continue-on-error: true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Upload coverage after failed tests.
continue-on-error only makes a failed upload non-fatal; it does not run this step after make test-race fails. Add if: ${{ always() }} to meet the stated always-attempt behavior.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 89-89: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml around lines 88 - 93, Update the “Upload
coverage” step to include an always-run condition using always(), ensuring it
executes even when the preceding make test-race step fails while retaining its
existing non-fatal upload behavior.
|
|
||
| - name: Upload coverage | ||
| if: matrix.coverage | ||
| uses: codecov/codecov-action@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow lines around action uses:"
sed -n '70,125p' .github/workflows/test.yml 2>/dev/null || true
echo
echo "All uses in workflow:"
rg -n "^\s*uses:\s*[^#]+" .github/workflows/test.yml || true
echo
echo "Repository workflow files:"
git ls-files .github/workflows || trueRepository: ENTERPILOT/GoModel
Length of output: 2473
Pin GitHub Actions to immutable commit SHAs.
Version tags are mutable and can move after review; pin each action used in .github/workflows/test.yml to a full reviewed SHA while keeping a # vX.Y.Z comment for maintainability.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 89-89: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 1 file
.github/workflows/test.yml#L89-L89(this comment).github/workflows/test.yml#L113-L113.github/workflows/test.yml#L116-L116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml at line 89, Pin every GitHub Action reference in
.github/workflows/test.yml at lines 89, 113, and 116 to its full reviewed
immutable commit SHA, replacing the mutable version tags while retaining an
adjacent # vX.Y.Z version comment for maintainability.
Source: Linters/SAST tools
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Confidence Score: 5/5The change appears safe to merge with all test suites retained and PostgreSQL provisioned only where the repository’s test setup requires it. The unit target still receives its PostgreSQL URL and generates coverage.out, while the remaining suites do not consume that service and integration tests continue to create their own PostgreSQL and MongoDB containers.
 T-Rex LogsWhat T-Rex did
|
The suite brings PostgreSQL and MongoDB up in parallel goroutines and let `docker run` fetch each image as a side effect, so both pulls left at the same instant. Registries meter anonymous pulls per second, and ECR Public answered the second one with "toomanyrequests: Rate exceeded". An implicit pull also gets no retry of its own, so any transient registry error failed the whole suite before a test ran — which is the class of failure this branch set out to remove, not just the one registry's limit. Images are now fetched up front, one at a time, with a bounded backoff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/docker_test.go`:
- Around line 35-67: Add table-driven tests covering dockerPullImage’s immediate
success, retry-then-success, exhausted retries, and context cancellation
outcomes. Introduce a narrow injectable command runner and backoff seam for the
dockerPullImage flow so tests can simulate pull results and avoid Docker
execution or real sleeping, while preserving production behavior and existing
error propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 41308ea0-0769-44a9-9d83-3ab7fb373faa
📒 Files selected for processing (2)
tests/integration/docker_test.gotests/integration/main_test.go
| func dockerPullImages(ctx context.Context, images ...string) error { | ||
| for _, image := range images { | ||
| if err := dockerPullImage(ctx, image); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func dockerPullImage(ctx context.Context, image string) error { | ||
| var lastErr error | ||
|
|
||
| for attempt := 1; attempt <= dockerPullAttempts; attempt++ { | ||
| if _, err := runDocker(ctx, "pull", image); err == nil { | ||
| return nil | ||
| } else { | ||
| lastErr = err | ||
| } | ||
|
|
||
| if attempt == dockerPullAttempts { | ||
| break | ||
| } | ||
|
|
||
| log.Printf("Pull of %s failed (attempt %d/%d), retrying: %v", image, attempt, dockerPullAttempts, lastErr) | ||
| select { | ||
| case <-ctx.Done(): | ||
| return fmt.Errorf("pull %s: %w: last error: %v", image, ctx.Err(), lastErr) | ||
| case <-time.After(time.Duration(attempt) * dockerPullBackoff): | ||
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("pull %s after %d attempts: %w", image, dockerPullAttempts, lastErr) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add table-driven tests for pull outcomes.
Cover immediate success, retry-then-success, exhausted retries, and cancellation. Use a narrow injected command/backoff seam so these cases do not require Docker or real sleeps.
As per coding guidelines, “Add or update table-driven tests for behavior changes, covering … error handling.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/docker_test.go` around lines 35 - 67, Add table-driven
tests covering dockerPullImage’s immediate success, retry-then-success,
exhausted retries, and context cancellation outcomes. Introduce a narrow
injectable command runner and backoff seam for the dockerPullImage flow so tests
can simulate pull results and avoid Docker execution or real sleeping, while
preserving production behavior and existing error propagation.
Source: Coding guidelines
What was failing
Two CI runs died within six hours of each other, both before any test executed:
Service containers are pulled during job init, so a registry hiccup fails the job outright. The two runs hit different jobs —
30160708337took out Integration Tests,30171320448took out E2E Tests — because the failing job is just whichever leg loses the coin toss.Why it started
fbe18ff9(#586) attached a Postgres service to all four legs of thego-testsmatrix. Only one of them connects to it:GOMODEL_TEST_POSTGRES_URLis read solely byinternal/storage/sqlx/sqlxtest, which thetests/*suites never import.TestMain, ignoring the service entirely.So three jobs pulled a database they never opened, taking on the registry risk for no coverage.
The change
docker runs — go through the AWS ECR Public mirror of Docker Hub's official library rather than Docker Hub's anonymous endpoint.Job names are unchanged, so required status checks keep matching. No test is dropped or skipped: the PostgreSQL subtests still run against a real 18, the integration suite still against its own 16 and Mongo 7.
Verification
The mirrored images are the same images, not lookalikes:
Locally, against the mirrored images: integration
ok 25.8s, e2eok 7.0s, contractok 0.5s, plus the full pre-commit gate (test-race,lint,fix-check, perf guard).One honest caveat: the failure mode was a connection timeout rather than a 429, so moving off Docker Hub's anonymous endpoint is a well-established mitigation rather than a proven one. If it recurs on the unit job, the fallback is the runner's preinstalled PostgreSQL 16.14, which removes the registry from that job altogether.
Supersedes #592, which diagnosed the same root cause but pulled only E2E out of the matrix — the job that happened to fail in the run it looked at, rather than the three that never needed the service.
🤖 Generated with Claude Code
Summary by CodeRabbit