Fix testcontainers cleanup #2023
Conversation
|
WalkthroughThe changes update the development dependencies in the testcontainers package to newer versions. In the source code, a new asynchronous helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant Resource
participant logCleanup
TestSuite->>Resource: Use resource (e.g., start container)
Note over TestSuite,Resource: Test execution
TestSuite->>logCleanup: logCleanup("ResourceName", cleanupFn)
activate logCleanup
logCleanup->>Resource: cleanupFn()
Resource-->>logCleanup: Cleanup complete or error
logCleanup-->>TestSuite: Log details (order, concurrency, duration, errors)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal-packages/testcontainers/src/index.ts (1)
73-73: Fix empty object pattern.There's an empty object pattern
{}in the function parameters which is unnecessary.-const network = async ({}, use: Use<StartedNetwork>) => { +const network = async (_: {}, use: Use<StartedNetwork>) => {or
-const network = async ({}, use: Use<StartedNetwork>) => { +const network = async (_, use: Use<StartedNetwork>) => {🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
internal-packages/testcontainers/package.json(1 hunks)internal-packages/testcontainers/src/index.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
internal-packages/testcontainers/src/index.ts
[error] 73-73: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
internal-packages/testcontainers/package.json (1)
13-14: Version bump for testcontainers dependencies looks good.The update of testcontainers-related packages from
^10.13.1to^10.25.0aligns with the PR's goal of fixing testcontainers cleanup. This minor version update likely includes improvements to container management and cleanup mechanisms.Also applies to: 16-16
internal-packages/testcontainers/src/index.ts (6)
34-71: Well-designed cleanup logging mechanism.This new implementation significantly improves the observability of container cleanup operations. The
logCleanupfunction provides:
- Detailed timing information (start, end, duration)
- Cleanup order tracking
- Concurrency detection
- Structured error handling
- JSON-formatted logs for easier parsing
This will be valuable for diagnosing test container issues, especially in CI environments.
78-82: Good refactoring of network cleanup.The network cleanup now uses the central logging mechanism, providing consistency with other resource cleanups.
95-97: Appropriate timeout increase for container stopping.Increasing the container stop timeout from the default to 30 seconds is a good practice. This gives containers more time to shut down gracefully, potentially preventing issues in subsequent tests caused by lingering containers.
119-121: Good application of logCleanup to the Prisma client.Consistently applying the same logging pattern to database disconnection will help track database-related cleanup issues.
140-142: Consistent timeout for Redis container.The 30-second timeout for Redis container is consistent with other containers, ensuring a standardized approach to cleanup.
194-196: Electric container cleanup matches other containers.The electric container cleanup now follows the same pattern as other containers with the standardized 30-second timeout.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: "🧪 Unit Tests (run ${{ matrix.run }})" | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| run: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20] | ||
| steps: | ||
| - name: 🔧 Configure docker address pool | ||
| run: | | ||
| CONFIG='{ | ||
| "default-address-pools" : [ | ||
| { | ||
| "base" : "172.17.0.0/12", | ||
| "size" : 20 | ||
| }, | ||
| { | ||
| "base" : "192.168.0.0/16", | ||
| "size" : 24 | ||
| } | ||
| ] | ||
| }' | ||
| mkdir -p /etc/docker | ||
| echo "$CONFIG" | sudo tee /etc/docker/daemon.json | ||
| sudo systemctl restart docker | ||
|
|
||
| - name: ⬇️ Checkout repo | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: ⎔ Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 8.15.5 | ||
|
|
||
| - name: ⎔ Setup node | ||
| uses: buildjet/setup-node@v4 | ||
| with: | ||
| node-version: 20.11.1 | ||
| cache: "pnpm" | ||
|
|
||
| # ..to avoid rate limits when pulling images | ||
| - name: 🐳 Login to DockerHub | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| username: ${{ secrets.DOCKERHUB_USERNAME }} | ||
| password: ${{ secrets.DOCKERHUB_TOKEN }} | ||
|
|
||
| - name: 📥 Download deps | ||
| run: pnpm install --frozen-lockfile | ||
|
|
||
| - name: 📀 Generate Prisma Client | ||
| run: pnpm run generate | ||
|
|
||
| - name: 🧪 Run Webapp Unit Tests | ||
| run: pnpm run test:webapp | ||
| env: | ||
| DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres | ||
| DIRECT_URL: postgresql://postgres:postgres@localhost:5432/postgres | ||
| SESSION_SECRET: "secret" | ||
| MAGIC_LINK_SECRET: "secret" | ||
| ENCRYPTION_KEY: "secret" | ||
|
|
||
| - name: 🧪 Run Package Unit Tests | ||
| run: pnpm run test:packages | ||
|
|
||
| - name: 🧪 Run Internal Unit Tests | ||
| run: pnpm run test:internal |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 19 hours ago
To fix the issue, we will add a permissions block at the root level of the workflow. This block will specify the minimal permissions required for the workflow to function correctly. Based on the tasks performed in the workflow, the contents: read permission is sufficient, as the workflow does not appear to modify repository contents or perform other actions requiring write access.
The permissions block will be added immediately after the name field at the top of the workflow file.
| @@ -1,2 +1,4 @@ | ||
| name: "🐳 Testcontainers" | ||
| permissions: | ||
| contents: read | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/testcontainers.yml (1)
1-78: 💡 Verification agent🧩 Analysis chain
Well-structured parallel testing workflow with Docker configuration
This new workflow efficiently runs tests in parallel with 20 matrix runs, which should significantly improve test execution time. The Docker address pool configuration matches the one added to the unit-tests workflow, ensuring consistent container networking.
A few recommendations:
- Add GitHub token permissions to limit the scope of the workflow
- Fix the trailing spaces in the YAML file for better code quality
Add a permissions block to limit token scope:
name: "🐳 Testcontainers" on: workflow_call: workflow_dispatch: push: +permissions: + contents: read jobs: unitTests: name: "🧪 Unit Tests (run ${{ matrix.run }})"And fix trailing spaces on lines 72 and 75:
SESSION_SECRET: "secret" MAGIC_LINK_SECRET: "secret" ENCRYPTION_KEY: "secret" - + - name: 🧪 Run Package Unit Tests run: pnpm run test:packages - + - name: 🧪 Run Internal Unit Tests
🌐 Web query:
Best practices for GitHub Actions token permissions💡 Result:
Best Practices for GitHub Actions Token Permissions
GitHub Actions provides an automated token (
GITHUB_TOKEN) that grants permissions to access resources within your repository. Managing these permissions properly is crucial for maintaining security in your CI/CD workflows. Here are the best practices for handling GitHub Actions token permissions:Follow the Principle of Least Privilege
The most fundamental best practice is to restrict
GITHUB_TOKENpermissions to only what's absolutely necessary for each workflow:
- Set read-only permissions by default at the repository or organization level[4][6]
- Explicitly define permissions needed for specific jobs or workflows[5]
- Avoid using the default permissions which might be overly permissive[7]
For example, if your workflow only needs to read repository contents, you should explicitly set:
permissions: contents: readConfigure Default Settings at Organization Level
For enterprises and organizations, the most effective approach to implement least privilege is to:
- Configure "Read repository contents and package permissions" as the default workflow permissions at the organization level[4]
- This ensures all GitHub Actions workflows have only read permissions for contents and packages by default
- Individual workflows can then request additional permissions as needed[6]
Use Job-Specific Permissions
When different jobs within a workflow need different permission levels, set them at the job level rather than the workflow level:
jobs: read-only-job: permissions: contents: read runs-on: ubuntu-latest steps: # Steps that only need to read content deployment-job: permissions: contents: write deployments: write runs-on: ubuntu-latest steps: # Steps that need to write content or create deploymentsThis approach ensures each job has only the permissions it specifically requires[5].
Be Cautious with Third-Party Actions
When using actions created by third parties, it's particularly important to restrict permissions:
- Limit the
GITHUB_TOKENpermissions when using third-party actions[5]- Consider using a more restrictive set of permissions for workflows that include third-party code[7]
For example:
permissions: actions: none checks: read contents: readAvoid Using Personal Access Tokens When Possible
The
GITHUB_TOKENis preferable to personal access tokens (PATs) in most cases because:
- It's automatically created and expired for each job[1]
- It has built-in scope limitations to the repository where the workflow runs[2]
- It's easier to audit and manage across an organization[4]
When you do need a PAT, create fine-grained tokens with limited repository access and specific permissions[8].
Implement Additional Security Controls
Beyond permission settings, consider these additional security controls:
- Use the StepSecurity Harden-Runner to restrict egress network traffic and implement runtime security controls[4]
- Monitor workflow runs for unexpected permission usage or suspicious activities
- Set expiration for any tokens that you create manually[8]
Monitor and Audit Token Usage
Regularly review:
- The permissions being used in your workflows
- The build logs of workflow runs (look at the 'Set up job' step to check
GITHUB_TOKENpermissions)[4]- Any changes to workflows that might modify permission scopes
By implementing these best practices, you can significantly reduce the security risks associated with GitHub Actions workflows while ensuring they have the necessary permissions to function properly.
Citations:
- 1: https://docs.github.com/actions/security-guides/automatic-token-authentication
- 2: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions
- 3: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token
- 4: https://www.stepsecurity.io/blog/github-actions-security-best-practices
- 5: https://dev.to/pwd9000/fgjgghjgh-19ka
- 6: https://www.stepsecurity.io/blog/github-token-how-it-works-and-how-to-secure-automatic-github-action-tokens
- 7: https://blog.gitguardian.com/github-actions-security-cheat-sheet/
- 8: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens
🏁 Script executed:
#!/bin/bash # Check for trailing whitespace in the workflow file rg -n '\s+$' .github/workflows/testcontainers.yml || trueLength of output: 71
Add least-privileged GitHub token permissions and remove trailing whitespace
To follow the principle of least privilege, explicitly scope
GITHUB_TOKENto only read repository contents, and clean up trailing spaces in the workflow file:• File:
.github/workflows/testcontainers.yml
– Insert apermissionsblock immediately after theon:section:name: "🐳 Testcontainers" on: workflow_call: workflow_dispatch: push: +permissions: + contents: read jobs: unitTests: …– Remove the two trailing-space lines detected (lines 72 and 75):
ENCRYPTION_KEY: "secret" - + - name: 🧪 Run Package Unit Tests run: pnpm run test:packages - + - name: 🧪 Run Internal Unit Tests🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 67-68: Basic Auth Credentials
(CKV_SECRET_4)
🪛 YAMLlint (1.35.1)
[error] 72-72: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
🪛 GitHub Check: CodeQL
[warning] 10-77: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🧹 Nitpick comments (1)
internal-packages/testcontainers/src/index.ts (1)
73-73: Fix empty object patternThe empty object pattern
{}should be replaced with a named parameter or removed for clarity.-const network = async ({}, use: Use<StartedNetwork>) => { +const network = async (_: {}, use: Use<StartedNetwork>) => {🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/testcontainers.yml(1 hunks).github/workflows/unit-tests.yml(1 hunks)internal-packages/testcontainers/src/index.ts(5 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/testcontainers.yml
[MEDIUM] 67-68: Basic Auth Credentials
(CKV_SECRET_4)
🪛 YAMLlint (1.35.1)
.github/workflows/testcontainers.yml
[error] 72-72: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
🪛 GitHub Check: CodeQL
.github/workflows/testcontainers.yml
[warning] 10-77: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 Biome (1.9.4)
internal-packages/testcontainers/src/index.ts
[error] 73-73: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (27)
- GitHub Check: 🧪 Unit Tests (run 20)
- GitHub Check: 🧪 Unit Tests (run 19)
- GitHub Check: 🧪 Unit Tests (run 18)
- GitHub Check: 🧪 Unit Tests (run 17)
- GitHub Check: 🧪 Unit Tests (run 16)
- GitHub Check: 🧪 Unit Tests (run 15)
- GitHub Check: 🧪 Unit Tests (run 14)
- GitHub Check: 🧪 Unit Tests (run 13)
- GitHub Check: 🧪 Unit Tests (run 12)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: 🧪 Unit Tests (run 11)
- GitHub Check: 🧪 Unit Tests (run 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: 🧪 Unit Tests (run 9)
- GitHub Check: 🧪 Unit Tests (run 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: 🧪 Unit Tests (run 7)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: 🧪 Unit Tests (run 6)
- GitHub Check: 🧪 Unit Tests (run 5)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: 🧪 Unit Tests (run 4)
- GitHub Check: 🧪 Unit Tests (run 3)
- GitHub Check: 🧪 Unit Tests (run 2)
- GitHub Check: 🧪 Unit Tests (run 1)
🔇 Additional comments (5)
.github/workflows/unit-tests.yml (1)
11-27: Docker configuration enhances testing reliabilityAdding this Docker address pool configuration is a good way to prevent network address conflicts when running containers during tests. This approach helps ensure consistent test environments and reduces potential flaky tests due to networking issues.
Consider adding a brief comment explaining why this configuration is necessary for better context for future developers.
internal-packages/testcontainers/src/index.ts (4)
34-71: Great improvement to resource cleanup logging and error handlingThe new
logCleanupfunction is an excellent addition that addresses several important aspects of container cleanup:
- Structured JSON logging provides consistent format for analyzing cleanup patterns
- Tracking cleanup order and parallelism helps understand resource disposal sequence
- Error handling ensures cleanup failures are logged but don't interrupt the process
- Timing information helps identify slow cleanups that might need optimization
The implementation is thorough and well-documented.
79-79: Improved network cleanup with centralized error handlingThe network cleanup now uses the
logCleanupfunction, which provides consistent error handling and logging. This is an improvement over the previous implementation.
93-93: Increased container stop timeout improves cleanup reliabilityIncreasing the container stop timeout from 10 to 30 seconds is a good change that gives containers more time to shut down gracefully. This helps prevent potential issues with lingering containers and improves test stability.
Also applies to: 134-134, 186-186
115-115: Enhanced Prisma client cleanupUsing
logCleanupfor Prisma client disconnection provides consistent error handling and logging, aligning with the overall cleanup strategy improvements in this PR.
Summary by CodeRabbit
New Features
Chores