chore(ci): pre-pull test container images to prevent dockerhub rate limiting when running tests

[ericallam]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: e8e28e6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Walkthrough

This pull request adds a new GitHub Actions workflow step titled "Pre-pull testcontainer images" to three workflow files: unit-tests-internal.yml, unit-tests-packages.yml, and unit-tests-webapp.yml. The step runs conditionally when the DOCKERHUB_USERNAME environment variable is set, and pre-pulls five Docker images (postgres:14, clickhouse-server:25.4-alpine, redis:7-alpine, testcontainers/ryuk:0.11.0, electricsql/electric:1.2.4) before dependency installation. The same pattern is applied consistently across all three workflows, adding 11 lines to each file.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is completely empty, missing all required template sections including issue reference, checklist, testing details, and changelog information.	Add a complete pull request description following the repository template, including the issue number, checklist items, testing steps, and a changelog entry explaining the change.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a CI step to pre-pull Docker images to prevent DockerHub rate limiting during test execution.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49df40c and e8e28e6.

📒 Files selected for processing (3)

• .github/workflows/unit-tests-internal.yml
• .github/workflows/unit-tests-packages.yml
• .github/workflows/unit-tests-webapp.yml

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)

• GitHub Check: Cursor Bugbot
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
• GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
• GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
• GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
• GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
• GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
• GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
• GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
• GitHub Check: typecheck / typecheck
• GitHub Check: Analyze (javascript-typescript)

🔇 Additional comments (3)

.github/workflows/unit-tests-internal.yml (1)

75-85: All five Docker images are required and actively used by internal tests.

The images are referenced in internal-packages/testcontainers/src/utils.ts and internal-packages/testcontainers/src/clickhouse.ts:

• postgres:14 — PostgreSQL container for database tests
• clickhouse/clickhouse-server:25.4-alpine — ClickHouse container for analytics
• redis:7-alpine — Redis container for cache/queue tests
• electricsql/electric:1.2.4 — Electric sync service for replication tests
• testcontainers/ryuk:0.11.0 — Testcontainers cleanup manager

The pre-pull approach is correct and efficient for the 8 parallel shards.

.github/workflows/unit-tests-packages.yml (1)

75-85: Verify image usage in package tests: three of five pre-pulled images are explicitly used.

The workflow pre-pulls five images, but only three are explicitly referenced in the package test code:

• ✓ postgres:14 (used in internal-packages/testcontainers/src/utils.ts)
• ✓ clickhouse/clickhouse-server:25.4-alpine (used in ClickHouseContainer)
• ✓ electricsql/electric:1.2.4 (used with explicit SHA256 digest)

However, redis:7-alpine is not explicitly invoked. The RedisContainer from @testcontainers/redis uses its default image, which may differ from redis:7-alpine. Additionally, testcontainers/ryuk:0.11.0 is the testcontainers library's internal cleanup image, automatically managed and not explicitly used by the package tests themselves.

Consider either removing these two images from the pre-pull or verifying they are necessary for the specific testcontainers versions in use.

.github/workflows/unit-tests-webapp.yml (1)

75-85: All five images are required for the webapp test suite.

The @internal/testcontainers package is actively used by webapp tests:

• containerTest uses postgres:14 and clickhouse/clickhouse-server
• containerWithElectricAndRedisTest uses electricsql/electric and redis:7-alpine
• redisTest uses redis:7-alpine
• testcontainers/ryuk is automatically used by the testcontainers framework for container cleanup

Examples: runsRepository.part1.test.ts, runsBackfiller.test.ts, and realtimeClient.test.ts all import container test utilities that depend on these images.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

PR Review: Pre-pull testcontainer images

Overall Assessment: ✅ This is a well-structured, pragmatic fix for DockerHub rate limiting issues. The changes are clean and follow the existing patterns in the workflows.

Code Quality and Best Practices

Strengths:

• Consistent implementation across all three workflow files
• Good use of the existing DOCKERHUB_USERNAME conditional pattern
• Helpful echo statements for debugging/visibility in logs
• Proper placement after DockerHub login to leverage authenticated session

Suggestions:

1. Consider parallel pulls: Docker can pull images in parallel. You could optimize this slightly with:

   docker pull postgres:14 &
   docker pull clickhouse/clickhouse-server:25.4-alpine &
   docker pull redis:7-alpine &
   docker pull testcontainers/ryuk:0.11.0 &
   docker pull electricsql/electric:1.2.4 &
   wait

   This would reduce the total pull time, though it may not be critical for CI.

2. Image version pinning: The images use specific tags (good!), but consider whether some should be pinned more strictly (e.g., using SHA digests) for reproducibility. That said, the current approach matches what testcontainers likely expects.

Potential Bugs or Issues

• No major issues identified. The conditional check (if: ${{ env.DOCKERHUB_USERNAME }}) correctly guards the step.

• Minor consideration: If a pull fails (e.g., image doesn't exist, network issue), the workflow will fail. This is probably the desired behavior, but you could add || true if you want the workflow to continue and let testcontainers retry later.

Performance Considerations

• Adds ~30-60 seconds to each workflow run due to sequential image pulls. This is a reasonable trade-off for avoiding rate limit failures.
• The internal and webapp workflows run 8 shards in parallel, meaning 8 runners will each pull these images. Consider whether GitHub Actions caching or a shared image cache could help, though this might be overkill for the current setup.

Security Concerns

• No security issues. The step only runs when DockerHub credentials are available, and it pulls from trusted sources (official images and well-known projects).
• The images pulled are appropriate for testing infrastructure (Postgres, Redis, ClickHouse, Electric, Ryuk).

Test Coverage

• This is a CI infrastructure change, so traditional unit tests don't apply.
• Recommendation: After merging, monitor the next few CI runs to confirm:
  1. The pre-pull step completes successfully
  2. Tests no longer hit rate limiting issues
  3. There's no significant impact on overall CI duration

Additional Observations

1. DRY opportunity: The same 11-line block is duplicated across three files. If you anticipate more changes to this list, you could extract it to a composite action. However, for a simple, stable list like this, duplication is acceptable.

2. Documentation: The PR description is empty. Consider adding a brief note about the rate limiting issue this solves for future reference.

3. Different test workflows may need different images: Currently all three workflows pull the same 5 images. If packages tests don't use all of them (e.g., Electric or ClickHouse), you could optimize by pulling only what's needed per workflow. But this is a minor optimization.

Verdict

Approve - This is a straightforward, practical fix that addresses a real CI reliability issue. The implementation is clean and follows existing patterns. The suggestions above are minor improvements, not blockers.

[vibe-kanban-cloud]

Review Complete

Your review story is ready!

View Story

Comment !reviewfast on this PR to re-generate the story.