chore: setup Dockerfile & Docker Compose for project-wide testing#1465
Conversation
|
WalkthroughThis pull request introduces new Docker configuration files to streamline testing. A Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant DC as Docker Compose
participant Container as Docker Container
Dev->>DC: Run 'docker compose up'
DC->>Container: Build image using Dockerfile (multi-stage)
Container->>Container: Run 'npm ci --ignore-scripts' (dependency install)
Container->>Container: Run 'npm test' (and optionally 'npm run lint')
Container-->>DC: Return test results
DC-->>Dev: Display test results
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (2)
docker-compose.yaml (1)
1-7: Docker Compose Service Configuration
Thetestservice is clearly defined and aligns with the new Dockerized testing workflow. A couple of suggestions:
- Verify Compatibility: Ensure that the
pull_policy: buildsetting is supported by the specific Docker Compose version you are using. In some contexts, this might need to be defined within abuildblock or adapted to the expected schema.- Context Clarity: Since both
imageandbuildare provided, double-check that the intended build process is triggered as expected and that there’s no ambiguity in which definition takes precedence.Development.md (1)
53-53: Clarification Note for Docker Testing
The additional note on line 53 clearly explains that tests run in a clean environment with freshly installed dependencies. This is useful for user understanding. Consider adding an optional troubleshooting tip or reference link for users who might not be familiar with Docker environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.dockerignore(1 hunks)Development.md(2 hunks)Dockerfile(1 hunks)docker-compose.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
🔇 Additional comments (4)
.dockerignore (1)
1-38: .dockerignore File Review
The patterns in the.dockerignorefile are comprehensive, covering common Node.js artifacts (likenode_modules, log files, and environment files), build artifacts, Git files, IDE configurations, OS-generated files, and test outputs. This will help keep the Docker build context lean and efficient.Dockerfile (1)
1-39: Multi-Stage Dockerfile for Optimized Testing
- The multi-stage build is well-structured: the
installerstage usesturbo pruneto extract only the necessary package files and source code, which is a smart technique for reducing build context size and enhancing caching.- Using
npm ciin thefinalstage ensures a clean, reproducible installation of dependencies based solely on the pruned files.- The final command,
CMD ["npm", "test"], aligns perfectly with the intended testing workflow.
Overall, this Dockerfile is well-designed to balance build efficiency with the need for a consistent testing environment.Development.md (2)
50-51: Simplified Docker Compose Command Update
The change to usedocker compose upsimplifies the testing process and leverages the new service configurations. This reduces complexity for users who previously had to deal with more cumbersomedocker runcommands.
180-180: Additional Debugging Guidance
Encouraging users to open an issue if persistent problems arise is a good proactive step to support troubleshooting and community engagement. Ensure that your issue template is prepared to capture the necessary details for Docker-related problems.
nightknighto
changed the title
|
Regarding SonarCube notes, I believe the notes aren't significant due to this Docker workflow being used only for testing, and not production. |
|
@derberg What do you think about the new dev workflow? |
|
the problem is that SonarCloud report is red and blocking merging and actually, Dockerfile is not only used by us for testing, it is also used by people that still use |
|
nevermind about my comment about Dockerfile, now I get you don't want to use https://github.com/asyncapi/generator/blob/master/apps/generator/Dockerfile but have one for entire monorepo for dev purpose only |
derberg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile (2)
7-10: Installer Stage – Copying Project Files
The installer stage copies the entire project into the container viaCOPY . ., preparing for subsequent pruning. Make sure your.dockerignoreis correctly configured (e.g., to excludenode_modulesand other non-essential files) so that the build context stays lean.
12-19: Turbo Prune Optimization
The commandRUN npx [email protected] prune @asyncapi/generator @asyncapi/template-js-websocket-client @asyncapi/generator-components --docker --out-dir /outeffectively reduces the project to just the needed
package.jsonfiles and related artifacts. It would be beneficial to document (perhaps via an inline comment or in project documentation) why usingnpxhere is sufficient—clarifying that it auto-installs the required version when missing—and to ensure that the pinned version remains compatible with future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (5)
Dockerfile (5)
1-3: Base Image and Working Directory Setup
The Dockerfile usesnode:18-alpineas a slim base image and sets the working directory to/app, which is clear and follows best practices.
23-30: Dependency Installation and Caching Strategy
The second stage copies over the prunedpackage.jsonandpackage-lock.jsonfiles, then runsnpm ci --ignore-scripts. This approach leverages Docker caching to ensure faster dependency installation while maintaining consistency via the lock file.
32-34: Copying the Remaining Source Code
Copying the full source from/out/full/ensures that only the essential files—filtered through the prune process—are included in the final image. This helps in keeping the image size minimal and aligns well with the caching strategy.
35-36: Setting Correct File Permissions
Changing the ownership of/appto thenodeuser viaRUN chown -R node:node /appis a solid security practice, ensuring that the application does not run as root.
38-41: Non-Root Execution and Default Command Configuration
Switching to the non-rootnodeuser and setting the default command tonpm testare both in line with the PR objectives. These steps simplify the testing workflow while enhancing container security.
Co-authored-by: Lukasz Gornicki <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
🧰 Additional context used
🪛 Hadolint (2.12.0)
Dockerfile
[error] 10-10: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (8)
Dockerfile (8)
1-4: Base Stage Initialization:
The base image is properly set to Node 18 on Alpine, and the working directory is configured as/app. This establishes a solid foundation for the multi-stage build.
12-20: Turbo Prune Command Execution:
TheRUN npx [email protected] prune ...command efficiently prunes the project down to only the necessarypackage.jsonfiles and source code, optimizing the Docker caching mechanism. Just ensure that the package names and the turbo version remain up to date as the project evolves.
21-28: Dependency Setup in the Final Stage:
Copying the prunedpackage.jsonfiles andpackage-lock.jsonfrom the installer stage is correctly implemented. This approach leverages Docker caching effectively and minimizes reinstallation overhead during dependency installation.
29-31: Optimized Dependency Installation:
Usingnpm ci --ignore-scriptsensures a clean and reproducible installation of dependencies from the copied package files. This not only speeds up builds but also adds a layer of security by ignoring unwanted scripts during installation.
32-34: Copying Full Source Code:
The commandCOPY --from=installer /out/full/ .successfully brings over the full source code after dependencies are installed. This separation of concerns aids in efficient caching and clear organization of the build process.
35-37: File Ownership Adjustment:
TheRUN chown -R node:node /appcommand correctly changes the ownership of the/appdirectory to thenodeuser, ensuring appropriate file permissions when running the container as a non-root user.
38-39: Non-Root User Execution:
The directiveUSER nodeis an excellent security best practice, enforcing that the container runs with non-root privileges.
41-42: Default Command Configuration:
SettingCMD ["npm", "test"]as the default command aligns perfectly with the project’s testing objectives. This ensures that tests are run by default, which is key to the streamlined Dockerized testing workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
9-11: Enhance COPY Command with Inline NOSONAR and Trailing Slash
The COPY command is currently split from its NOSONAR directive and lacks a trailing slash on the destination directory. To satisfy Hadolint rule DL3021 and ensure Sonar warnings are properly suppressed, consider combining them as follows:-# NOSONAR -COPY . . +COPY . ./ // NOSONARThis update explicitly attaches the NOSONAR directive to the command and clarifies the destination directory structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (4)
Dockerfile (4)
1-3: Base Image and Working Directory Setup
The Dockerfile correctly starts with a lightweight Node.js 18 Alpine base image and properly sets the working directory to/app. This provides a solid base for subsequent build stages.
20-20: Prune Command with NPX for Package Optimization
Usingnpx [email protected] prune ...effectively prunes the project to the minimal required files for dependency management. NPX will automatically handle the installation if the specified version isn’t already available. Just ensure that the fixed version remains in line with project standards over time.
26-31: Optimized Dependency Installation for Caching
By copying only the prunedpackage.jsonfiles (and the correspondingpackage-lock.json) before runningnpm ci --ignore-scripts, the Dockerfile smartly leverages caching to avoid redundant installations. This approach improves build speed and consistency.
33-42: Ensuring Secure and Consistent Final Image
The final build stage is well-structured: it copies the complete source code, adjusts the ownership to the non-rootnodeuser, and sets the default command to runnpm test. Running the container as a non-root user enhances security, and coupling this with the testing command aligns with the Dockerized testing workflow goals.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Dockerfile (3)
1-3: Solid Base Image and Workdir Setup.
Lines 1–3 correctly use a lightweightnode:18-alpineimage and set the working directory to/app. This establishes a proper base for subsequent stages. Optional: Consider setting an environment variable (for example,ENV NODE_ENV test) if tests need to run under a specific environment.
15-22: Effective Use of Turbo Prune for Monorepo Organization.
Lines 15–22 use theRUN npx [email protected] prune ...command to slim down the project to only the necessary package-related files. This is a smart approach to leverage caching by isolating the dependency files. Remember that the list of package names is hardcoded; ensure you update these as your monorepo evolves.
41-44: Adherence to Security Best Practices with Non-root User and Clear Default Command.
Lines 41–44 switch the running user tonode(ensuring the container does not run as root) and set the default command to run tests viaCMD ["npm", "test"]. This matches the PR’s testing focus. Optionally, setting an explicit environment variable (such asENV NODE_ENV test) here could further clarify the container’s role.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Dockerfile (1)
Learnt from: nightknighto
PR: asyncapi/generator#1465
File: Dockerfile:0-0
Timestamp: 2025-04-09T14:19:06.716Z
Learning: In Dockerfiles, comments must begin with the `#` character and must be on their own line. Inline comments (like `// NOSONAR`) are not supported in Dockerfile syntax and will be interpreted as part of the command. SonarQube suppressions in Dockerfiles should be done using a separate comment line with the `#` prefix.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (3)
Dockerfile (3)
5-13: Clear and Commented Installer Stage Initialization.
Lines 5–13 articulate the intent behind the installer stage. The comments explain why the entire project is copied (i.e. for the subsequentturbo pruneprocess) and underscore that this is primarily for local development and testing. Just ensure that a corresponding.dockerignorefile is in place to exclude unnecessary files (such asnode_modules) and speed up the build.
24-31: Optimized Dependency Extraction in Final Stage – Part 1.
Lines 24–31 transition into the final build stage. Copying the prunedpackage.jsonfiles andpackage-lock.jsonbefore runningnpm ci --ignore-scriptsis an effective use of caching that minimizes unnecessary reinstallation of dependencies when unrelated source files change.
32-39: Efficient Final Stage Execution and Security Enhancements.
Lines 32–39 execute the dependency installation and then copy over the remaining source code. Runningnpm ci --ignore-scriptsguarantees a clean, reproducible dependency tree, and copying the rest of the source afterward leverages cache layers effectively. Thechown -R node:node /appcommand on line 39 is a good security practice to ensure proper file ownership.
derberg
left a comment
derberg
left a comment
There was a problem hiding this comment.
please look into the chat we had in slack.
wdyt?
|
Added running lint optionally based on an environment variable
Lint running after tests: Lints fails though, but I believe that is an unrelated problem: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docker-compose.yaml (1)
1-7: Solid Docker Compose setup for testing.The configuration is clear and effective for local testing. The use of conditional linting via shell expansion is clever and keeps the workflow simple. Consider adding a brief comment or documentation note explaining the
${LINT:+&& npm run lint}syntax for maintainability, especially for contributors less familiar with shell parameter expansion.If you ever need to persist test results or coverage reports, consider mounting a volume to the host.
Development.md (1)
53-55: Fix unordered list indentation for Markdown compliance.The list items for Windows and Linux/macOS are indented with three spaces, which is non-standard and flagged by markdownlint. Use zero or two spaces for proper Markdown rendering and linter compliance.
- - Windows: `set LINT=true && docker compose up` - - Linux/macOS: `LINT=true docker compose up` + - Windows: `set LINT=true && docker compose up` + - Linux/macOS: `LINT=true docker compose up`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
54-54: Unordered list indentation
Expected: 0; Actual: 3(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 3(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Development.md(2 hunks)docker-compose.yaml(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
Development.md
54-54: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (1)
Development.md (1)
50-57: Clear and concise Docker testing instructions.The updated instructions for Docker-based testing are accurate and easy to follow. The explanation of the
LINTvariable and platform-specific examples are helpful for contributors.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
54-54: Unordered list indentation
Expected: 0; Actual: 3(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 3(MD007, ul-indent)
Co-authored-by: Lukasz Gornicki <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Development.md (1)
57-57: Use a native markdown note instead of a blockquote for important information.Replace the blockquote with a native markdown note for better visibility and consistency, as previously suggested in review comments.
-> This approach ensures a clean environment for each test run by cleanly installing dependencies and running tests in a Docker container. +> [!NOTE] +> This approach ensures a clean environment for each test run by cleanly installing dependencies and running tests in a Docker container.
🧹 Nitpick comments (1)
Development.md (1)
54-55: Fix unordered list indentation for platform-specific lint instructions.The indentation for the unordered list items under the linting instructions does not follow markdown best practices and is flagged by static analysis. Remove the extra spaces to align with the parent list.
- - Windows: `set LINT=true && docker compose up` - - Linux/macOS: `LINT=true docker compose up` + - Windows: `set LINT=true && docker compose up` + - Linux/macOS: `LINT=true docker compose up`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
54-54: Unordered list indentation
Expected: 0; Actual: 3(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 3(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Development.md(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
Development.md
54-54: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (1)
Development.md (1)
50-51: Docker Compose command is clear and concise.The new instruction to use
docker compose upsimplifies the workflow and improves usability.
|
|
/rtm |
3 participants
Description
This PR improves the Dockerized testing workflow by simplifying the command and making use of caching.
docker compose up.Related issue(s)
Fixes #1423
Screenshots
...
jsandmdfiles then re-tested.Notice:
RUN npm ciis cached, while changed files were only reflected in the next line.Design Choices
Writing the dockerfile in a correct way to cache npm modules was the most difficult part of the PR, due to the monorepo structure. Some possible ways were:
1- Manually copying each package.json file from the project. Ex:
Problem: While manually copying package.json of each package could be ok, the folder structure of the package.json must be written manually and would need manual edit if changed. This problem is very apparent in the js websocket client, which has a deeply nested package.json. Refused this approach.
2- Using
turbo prunecommand. Turborepo CLI has a command that can prune a project out of modules and divide it into a folder of package.json with respecting the nested structure, and a folder of source code. The first folder will be used to install npm modules, and the second will be to add source code.A small problem is that the command requires a scope (a package to prune as a target), and there is no
alloption, so we have to specify all package names. Some packages include other packages as dependencies so some names can be skipped. I've opened an issue anyways in their github: vercel/turborepo#10267This approach gave great results, and is the chosen one.
Summary by CodeRabbit
.dockerignorefile to exclude unnecessary files from container builds, enhancing efficiency.