test: test-project switch from mounting to building image with docker#1482
Conversation
|
WalkthroughThis set of changes updates the Docker-based testing workflow for the generator's test project. The Dockerfile now accepts a configurable Node.js version via build arguments. The Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Docker Compose
participant Docker Engine
participant Container (test-project)
Developer->>Docker Compose: Run test-project service
Docker Compose->>Docker Engine: Build image (with NODE_VERSION)
Docker Engine->>Container (test-project): Start container with /app context
Container (test-project)->>Container (test-project): Run test.sh in /app
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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:
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)
apps/generator/test/test-project/Dockerfile (1)
5-8: Copying Project Context and Script ExecutionThe
COPY . /appinstruction leverages the build context provided by the accompanying.dockerignorefile to omit unnecessary files. TheCMDcorrectly specifies running the test script (test.sh) with a parameter. If further optimization is needed down the line (for example, reducing image size or adopting multi-stage builds), consider this as an area for optional refactoring.apps/generator/test/test-project/docker-compose.yml (1)
11-12: Assessing Privileged Mode UsageThe
test-projectservice is configured withprivileged: true. Verify that this elevated permission is strictly necessary for the tests to run correctly, as it may introduce security risks. If possible, consider reducing privileges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.dockerignore(1 hunks).gitattributes(1 hunks)apps/generator/test/test-project/Dockerfile(1 hunks)apps/generator/test/test-project/docker-compose.yml(1 hunks)apps/generator/test/test-project/test-entrypoint.sh(0 hunks)
💤 Files with no reviewable changes (1)
- apps/generator/test/test-project/test-entrypoint.sh
⏰ 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 (5)
.gitattributes (1)
1-3: Enforcing Consistent LF Line EndingsThe file correctly ensures that all shell script files (with extensions
.shand.bash) use LF line endings. This is a good practice for avoiding cross-platform issues with script execution.apps/generator/test/test-project/Dockerfile (1)
1-2: Proper Use of Build Arguments and Base ImageDefining
ARG NODE_VERSION=18and using it in theFROM node:${NODE_VERSION}instruction provides flexibility in selecting the Node.js version at build time.apps/generator/test/test-project/docker-compose.yml (2)
13-17: Transition to Build ConfigurationSwitching from using a pre-built image to a build directive is well implemented. The build context is set to the project root, the correct Dockerfile is referenced, and the
NODE_VERSIONargument leverages the${NODE_IMAGE_TAG}value. Ensure that the.dockerignorefile further refines the build context.
21-24: Network Configuration ConsistencyThe network settings using the bridge driver are maintained consistently. This configuration should work well in your test environment.
.dockerignore (1)
1-38: Comprehensive Exclusion PatternsThe
.dockerignorefile effectively excludes directories and files that are not necessary for the Docker build context (e.g., node modules, logs, environment files, IDE settings, OS-specific files, and test artifacts). This will optimize the image build process by reducing the build context size.
|
Let's put this PR on hold till #1465 is solved. I will probably re-use additions from it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/generator/test/test-project/docker-compose.yml (1)
24-24: Nitpick: Explicit network driver is redundant
By default, Compose uses thebridgedriver. You can safely omitdriver: bridgeunless you need to override settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(1 hunks)apps/generator/test/test-project/docker-compose.yml(1 hunks)apps/generator/test/test-project/test.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
apps/generator/test/test-project/test.sh
[warning] 54-54: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ 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 - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (2)
Dockerfile (1)
1-2: Parameterize Node.js version via build argument
Good use ofARG NODE_VERSIONto make the base image configurable. Ensure you update any documentation or CI scripts to document the newNODE_VERSIONbuild‑arg.Do you have a README update or CI step to pass a custom
NODE_VERSION? Verify that builds without explicitly passing--build-arg NODE_VERSIONstill behave as expected.apps/generator/test/test-project/docker-compose.yml (1)
12-18: Verify build-based setup and environment variables
Switching from volume mounts to a build context is solid. Please confirm:
- Your CI or local environment defines
NODE_IMAGE_TAG, otherwiseNODE_VERSIONmay default unexpectedly.- The
pull_policy: buildfield is supported by your Compose version.- Running the container as
rootis intentional—consider reverting to a non‑root user post‑build for security.
|
|
||
| # Always run these steps | ||
| cd app | ||
| cd /app |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential cd /app failure
The new absolute path change is correct, but without error handling a failed cd could let subsequent commands run in the wrong directory.
Apply this diff to exit on failure:
- cd /app
+ cd /app || exit 1This aligns with ShellCheck SC2164.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cd /app | |
| - cd /app | |
| + cd /app || exit 1 |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 54-54: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
It's good in many things, but this suggestion definitely isn't 😂
…E_VERSION in Docker setup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/generator/test/test-project/README.md (2)
6-6: Use correct macOS capitalization.The official branding for Apple's operating system is
macOS, so update the label accordingly.Apply this diff:
-Linux/MacOS: `NODE_VERSION=18 docker-compose up --abort-on-container-exit --force-recreate`. +Linux/macOS: `NODE_VERSION=18 docker-compose up --abort-on-container-exit --force-recreate`.🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: The operating system from Apple is written “macOS”.
Context: ...e and run the following command: Linux/MacOS: `NODE_VERSION=18 docker-compose up --a...(MAC_OS)
8-8: Add spacing around the command separator for clarity.Including spaces around
&&improves readability and prevents parsing issues in some environments.Apply this diff:
-Windows: `set NODE_VERSION=18&& docker-compose up --abort-on-container-exit --force-recreate`. +Windows: `set NODE_VERSION=18 && docker-compose up --abort-on-container-exit --force-recreate`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pr-testing-with-test-project.yml(1 hunks)apps/generator/test/test-project/README.md(1 hunks)apps/generator/test/test-project/docker-compose.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/pr-testing-with-test-project.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/generator/test/test-project/docker-compose.yml
🧰 Additional context used
🪛 LanguageTool
apps/generator/test/test-project/README.md
[grammar] ~6-~6: The operating system from Apple is written “macOS”.
Context: ...e and run the following command: Linux/MacOS: `NODE_VERSION=18 docker-compose up --a...
(MAC_OS)
⏰ 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
|
@derberg Updated the PR:
Ready for review |
you mean custom entrypoint script? |
|
No, the removal of the script is an old change. I removed the Dockerfile created in an intermediary commit. |
|
derberg
left a comment
derberg
left a comment
There was a problem hiding this comment.
great stuff, just checked on local with podman, and works like a charm
long running of course 😄 but passing 😄
|
/rtm |
|
@allcontributors please add @nightknighto for test,infra |
|
I've put up a pull request to add @nightknighto! 🎉 |
3 participants
Description
In generator's test-project, convert mounting the project to building an image:
.gitattributesto enforce all scripts to be downloaded in LF line ending.Follow-up issues:
Related issue(s)
Fixes #1425
Screenshots
Tests passing
Summary by CodeRabbit
.dockerignoreto exclude Docker-related files from build context..gitattributesto enforce consistent line endings for shell scripts.NODE_IMAGE_TAGtoNODE_VERSIONin documentation and CI workflows.