chore(ci): do not include dev dependencies in docker images #37481
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughMake CI/node setup type-aware (development vs production); add dependency-merge/restore and bundle size-reduction cleanup in Meteor/docker build steps; convert Alpine Dockerfile to multi-stage; remove translation-check task and root Changes
Sequence Diagram(s)sequenceDiagram
participant CI as Workflow
participant Setup as setup-node action
participant Cache as Cache
participant Yarn as Yarn
participant Meteor as meteor-build action
rect rgb(245,250,255)
CI->>Setup: invoke (install: true, type: production)
end
Setup->>Cache: lookup node-modules:production:...:SOURCE_HASH-v8
alt cache hit
Cache-->>Setup: restore node_modules
else cache miss
Setup->>Yarn: run "yarn workspaces focus --all --production"
Yarn-->>Setup: node_modules populated
Setup->>Cache: save node-modules:production:...-v8
end
CI->>Meteor: start meteor build (production)
Meteor->>Meteor: merge deps -> apps/meteor/package.json (backup)
Meteor->>Yarn: cd apps/meteor && yarn workspaces focus --all --production
Meteor->>Meteor: remove swc/core, TS/babel paths and .d.ts files
Meteor->>Meteor: restore original apps/meteor/package.json
sequenceDiagram
participant BuildDocker as build-docker action
participant FS as Filesystem
Note over BuildDocker,FS: Pre-publish SWC pruning for rocketchat
BuildDocker->>FS: detect arch -> swc_arch
BuildDocker->>FS: list swc-core/core-*
BuildDocker->>FS: remove core-* except linux-${swc_arch}-gnu
FS-->>BuildDocker: sizes reduced
BuildDocker->>BuildDocker: continue publish/load
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
rodrigok
force-pushed
the
chore/build-with-production
branch
from
3fc4c6d to
8df22a1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/setup-node/action.yml (1)
76-84: Both install steps execute unconditionally wheninstall: true, defeating production mode logic.Currently, both the regular
yarnstep (line 76-79) andyarn workspaces focus --all --productionstep (line 81-84) share the sameif: inputs.installcondition. This means whenproduction: trueis set:
- The regular
yarninstalls all dependencies (including dev)- Then
yarn workspaces focus --all --productionruns but cannot retroactively remove already-installed dev dependenciesResult: Dev dependencies remain in
node_modulesdespiteproduction: true.The conditional logic should be:
- Regular install should only run when production mode is disabled:
if: inputs.install && inputs.production != true- Production install should only run when production mode is enabled:
if: inputs.install && inputs.production == true- name: yarn install - if: inputs.install + if: inputs.install && inputs.production != true shell: bash run: YARN_ENABLE_HARDENED_MODE=${{ inputs.HARDENED_MODE }} yarn - name: yarn install production - if: inputs.install + if: inputs.install && inputs.production == true shell: bash run: YARN_ENABLE_HARDENED_MODE=${{ inputs.HARDENED_MODE }} yarn workspaces focus --all --production
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/actions/meteor-build/action.yml(1 hunks).github/actions/setup-node/action.yml(3 hunks).github/workflows/ci.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- .github/actions/meteor-build/action.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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.github/actions/setup-node/action.yml (1)
15-18: Input definition looks good.The new
productioninput is properly defined with appropriate type and description.
rodrigok
force-pushed
the
chore/build-with-production
branch
2 times, most recently
from
8464929 to
13c5678
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37481 +/- ##
===========================================
+ Coverage 68.97% 68.99% +0.01%
===========================================
Files 3358 3358
Lines 114240 114240
Branches 20537 20537
===========================================
+ Hits 78798 78816 +18
+ Misses 33349 33341 -8
+ Partials 2093 2083 -10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
rodrigok
force-pushed
the
chore/build-with-production
branch
6 times, most recently
from
aac2fdb to
48d8992
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/actions/meteor-build/action.yml(1 hunks).github/actions/setup-node/action.yml(3 hunks).github/workflows/ci.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- .github/actions/meteor-build/action.yml
🔇 Additional comments (2)
.github/actions/setup-node/action.yml (2)
54-54: ✅ Cache key correctly differentiates by type.The cache key now includes
type-${{ inputs.type }}, ensuring development and production dependency trees maintain separate caches. This prevents cache pollution and aligns with the PR objective.Note: The syntax error flagged in a prior review (extra closing brace) appears to be resolved.
78-85: ✅ Production install correctly excludes dev dependencies.The conditional logic properly splits into two paths:
yarn installfor development (line 78)yarn workspaces focus --all --productionfor production (line 83-85)The
--productionflag on the production path correctly excludes dev dependencies, achieving the PR objective. The mutually exclusive conditions prevent accidental double-installs.
rodrigok
force-pushed
the
chore/build-with-production
branch
from
48d8992 to
8cb77db
Compare
rodrigok
force-pushed
the
chore/build-with-production
branch
from
8cb77db to
421ee5f
Compare
rodrigok
force-pushed
the
chore/build-with-production
branch
8 times, most recently
from
ce1dbfe to
52b89a7
Compare
rodrigok
force-pushed
the
chore/build-with-production
branch
16 times, most recently
from
ab695d5 to
428c82a
Compare
rodrigok
force-pushed
the
chore/build-with-production
branch
from
428c82a to
2a347fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/actions/build-docker/action.yml (1)
78-86: Minor: Typo in comment.Line 78 has "sized" which should be "size" ("reduce image size" not "reduce image sized").
- # Removes unnecessary swc cores to reduce image sized + # Removes unnecessary swc cores to reduce image size.github/actions/meteor-build/action.yml (1)
180-192: Improve error handling and logging in cleanup loop.The cleanup loop could fail silently or with unclear messages. Consider improving robustness:
- Current: Simply echoes "Path not found" if directory doesn't exist
- Risk: Typos in paths would be silently ignored, causing fewer modules to be removed than expected
Consider adding a summary at the end showing how many directories were actually removed vs. expected.
+ removed_count=0 + total_count=${#meter_modules_to_remove[@]} + for dir_path in "${meter_modules_to_remove[@]}"; do path=/tmp/dist/bundle/programs/server/npm/node_modules/${dir_path} if [ -d "$path" ]; then rm -rf "$path" + ((removed_count++)) echo "Removed directory: $path" else echo "Path is not a directory or does not exist: $path" fi done + + echo "Successfully removed $removed_count of $total_count expected directories"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/actions/build-docker/action.yml(1 hunks).github/actions/meteor-build/action.yml(3 hunks).github/actions/setup-node/action.yml(3 hunks).github/workflows/ci.yml(1 hunks)apps/meteor/.docker/Dockerfile.alpine(3 hunks)package.json(0 hunks)turbo.json(0 hunks)
💤 Files with no reviewable changes (2)
- package.json
- turbo.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/setup-node/action.yml
- .github/workflows/ci.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (9)
apps/meteor/.docker/Dockerfile.alpine (4)
1-19: Multi-stage build correctly achieves production-only image.The builder stage properly installs only production dependencies via
--omit=dev(line 12), and the final stage copies the built bundle from the builder without re-running npm install. Build tools (python3, make, g++) stay in the builder context and don't bloat the final image. This approach aligns with the PR goal of removing dev dependencies and should deliver the reported size reductions.
11-19: Verify sharp architecture-specific binary handling works as intended.The sharp reininstall logic (lines 15–19) appears designed to refresh architecture-specific native bindings in
@img, but the sequence—reinstalling sharp's dependencies within its own directory, then relocating@imgto the parent scope, then discarding sharp's entirenode_modules—is non-standard. Confirm that:
- The relocated
@imgfolder is correctly discovered and used by the sharp module at runtime.- The final image can successfully load sharp and its native bindings after this relocation.
Consider testing the built image against sharp-dependent functionality to ensure no runtime
ENOENTor binding-load errors occur.
39-44: User and package setup is sound.The shadow utility (line 39) is correctly used for non-root user creation (lines 43–44). The detailed comment block (lines 27–38) justifying the
nogroup→rocketchatremapping shows good security and compatibility reasoning. OpenSSL upgrade (line 42) and the explicit CVE reference are also appropriate for runtime security.
57-57: Correct use of COPY --from with ownership.The
COPY --from=builder --chown=rocketchat:rocketchatdirective (line 57) ensures the app bundle is owned by the non-root user, maintaining proper file permissions and reducing privilege escalation risk..github/actions/build-docker/action.yml (1)
78-86: Path structure verified—cleanup logic is sound.The hardcoded path
/tmp/build/bundle/programs/server/npm/node_modules/meteor/babel-compiler/node_modules/@meteorjs/swc-core/.swc/node_modules/@swcis correct. The meteor-build action removes specific arch-specific SWC binaries during the build phase, and the build-docker action completes the cleanup by removing any remainingcore-*directories not matching the target architecture. Both workflows reference the identical structure, confirming it exists as documented..github/actions/meteor-build/action.yml (4)
163-164: Clarify the purpose of package.json restoration before the build.The workflow merges devDependencies into dependencies, installs with
type='production', then restores the original package.json (with devDependencies listed) before building. This creates a mismatch:package.jsonlists devDependencies butnode_modulesdoesn't contain them.Verify that Meteor build doesn't rely on package.json matching node_modules contents, or explain why this restoration is necessary.
54-63: No issues found—setup-node action correctly supports type='production'.The verification confirms that the
.github/actions/setup-nodeaction properly accepts thetypeinput and implements the intended behavior: whentype='production'is passed, it runsyarn workspaces focus --all --production, which excludes devDependencies as expected. The mechanism used in this PR is correctly implemented.
45-52: ****The jq merge command at line 52 is safe. Analysis shows apps/meteor/package.json has 244 dependencies and 156 devDependencies with no overlapping packages. Since each package appears in only one list, the merge order is inconsequential, and no version conflicts can occur. The original concern about differing versions for the same package does not apply to this codebase.
168-197: The review comment contains misunderstandings about the cleanup logic.The cleanup happens after
yarn build:cicompletes successfully. This is post-build size optimization, not pre-build removal:
Platform-specific swc removal (lines 169-171): These removals target
meteor/babel-compiler/node_modules/@swc/core-*binaries from Meteor's internal build tools, not the runtime. Since the build already succeeded, these unused architecture binaries can safely be removed from the bundle. The cache key includes${{ runner.arch }}, so each architecture's build is cached separately.@babel handling (line 177): The commented removal is correct—root-level
@babelis intentionally preserved. The code removes onlymeteor/babel-compiler/node_modules/@babel(Meteor's build-time @babel), not the production@babelpackage needed at runtime..d.ts removal (line 196): Safe—these are build-time type definitions, verified by script showing no
@babel/runtimeimports in runtime code and no Minimongo dependency.
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
https://rocketchat.atlassian.net/browse/ARCH-1854
Reduces uncompressed image size from 3.4GB to 2.1GB (-1.3GB or 62% of the original size)
Reduces compressed image size from 601MB to 365MB (-236MB or 61% of the original size)
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Chores
Bug Fixes / Optimizations