chore(ci): Lint dockerfiles #16558
chore(ci): Lint dockerfiles #16558
Conversation
WalkthroughThe pull request includes modifications to several Dockerfiles across different services, enhancing clarity, efficiency, and functionality. Key updates involve the installation of the ICU analysis plugin in the Elasticsearch Dockerfile, improvements to package installation commands, and standardization of syntax in multiple Dockerfiles. The changes also include replacing Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16558 +/- ##
==========================================
- Coverage 36.77% 36.77% -0.01%
==========================================
Files 6853 6853
Lines 142181 142194 +13
Branches 40556 40559 +3
==========================================
+ Hits 52292 52295 +3
- Misses 89889 89899 +10
Flags with carried forward coverage won't be shown. Click here to find out more. see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
AndesKrrrrrrrrrrr
added
the
deprecated:automerge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
infra/scripts/Dockerfile (1)
21-22: LGTM! Efficient layer management.Good use of && to combine build commands, reducing the number of layers. Consider adding a comment explaining what each ncc build produces for better maintainability.
-RUN ./node_modules/.bin/ncc build src/feature-env.ts -o /app/dist/feature-env && \ - ./node_modules/.bin/ncc build src/secrets.ts -o /app/dist/secrets +# Build feature environment and secrets management utilities +RUN ./node_modules/.bin/ncc build src/feature-env.ts -o /app/dist/feature-env && \ + ./node_modules/.bin/ncc build src/secrets.ts -o /app/dist/secretsapps/services/search-indexer/dev-services/elasticsearch/Dockerfile (3)
Line range hint
4-4: Add error handling for plugin installation.While adding the ICU plugin is beneficial for Unicode support, consider adding error checking to ensure the installation succeeds.
-RUN bin/elasticsearch-plugin install analysis-icu +RUN bin/elasticsearch-plugin install analysis-icu || exit 1
Line range hint
6-7: Optimize repository configuration commands.The two
sedcommands can be combined into a single RUN instruction to reduce the number of layers in the final image.-RUN sed -i 's/mirrorlist/#mirrorlist/g' /etc/yum.repos.d/CentOS-* -RUN sed -i 's|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g' /etc/yum.repos.d/CentOS-* +RUN sed -i 's/mirrorlist/#mirrorlist/g' /etc/yum.repos.d/CentOS-* && \ + sed -i 's|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g' /etc/yum.repos.d/CentOS-*
14-14: Enhance security for external downloads.The GitHub download lacks checksum verification, which could pose a security risk. Consider adding SHA256 verification for the downloaded file.
+ARG DICT_SHA256="<expected-sha256-hash>" RUN wget -O master.zip https://github.com/island-is/elasticsearch-dictionaries/archive/master.zip +RUN echo "${DICT_SHA256} master.zip" | sha256sum -c -scripts/ci/Dockerfile (1)
138-139: Address TODO regarding awscli dependencyThe TODO comment indicates a large dependency (157 MB) that could be removed. This should be addressed to reduce the image size.
Would you like me to help create an issue to track the removal of the awscli dependency?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/services/search-indexer/dev-services/elasticsearch/Dockerfile (1 hunks)
- infra/scripts/Dockerfile (1 hunks)
- scripts/Dockerfile.proxy (1 hunks)
- scripts/ci/Dockerfile (10 hunks)
✅ Files skipped from review due to trivial changes (1)
- scripts/Dockerfile.proxy
🧰 Additional context used
📓 Path-based instructions (2)
apps/services/search-indexer/dev-services/elasticsearch/Dockerfile (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
scripts/ci/Dockerfile (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
🔇 Additional comments (9)
infra/scripts/Dockerfile (3)
3-3: LGTM! Good practices implemented.The changes follow Docker best practices:
- Using uppercase for stage names improves readability
- Using
--no-cachewithapk addprevents unnecessary package index caching- Proper use of hadolint ignore directive
Also applies to: 5-6
8-9: LGTM! Proper stage setup.Good practice to set WORKDIR early in the stage, providing clear context for subsequent commands.
24-29: LGTM! Clean final stage setup.Good practices observed:
- Proper use of COPY instead of ADD
- Clear WORKDIR setting
- Well-organized file copying from build stage
scripts/ci/Dockerfile (6)
13-16: Good use of COPY instead of ADD!The change from ADD to COPY is a best practice as it makes the intention more explicit and predictable. Also, using
--immutablewith yarn ensures reproducible builds.
Line range hint
29-37: Good environment variable configuration for buildsThe use of build arguments and environment variables follows best practices:
- Setting NODE_ENV=production
- Configuring NODE_OPTIONS for memory management
- Using build arguments for app configuration
104-111: Well-structured package updates and configurationGood practices observed:
- Package updates and upgrades are properly sequenced
- Environment extraction scripts are organized in appropriate locations
- Base path configuration is flexible
Line range hint
152-161: Good security practices with user permissionsExcellent handling of permissions:
- Files are properly owned by pwuser
- Playwright browser installation is user-specific
- Environment variables are well configured
Line range hint
67-88: Verify the need for --no-experimental-fetch flagThe flag
--no-experimental-fetchis used in the Express output but not in Next.js output. Please verify if this inconsistency is intentional.
Line range hint
114-127: Verify the empty Jest configurationThe empty Jest configuration (
module.exports = {};) might be too permissive. Consider explicitly defining the test environment and other relevant configurations.✅ Verification successful
Let me gather more information about the Jest configuration by checking the actual content of some key Jest configuration files.
Let me check the Jest configuration in package.json and nx.json since they might contain the project's base Jest configuration.
Let me check the actual Jest configuration files in the project to understand the test setup better.
Let me check the actual Jest configuration files in the project, focusing on the main configuration files.
Based on the analysis of the Jest configuration files, I can now provide a final response about the empty Jest configuration concern.
Empty Jest configuration is intentional and inherits from project preset
The empty Jest configuration in the Dockerfile (
module.exports = {};) is acceptable because:
The project uses a comprehensive base Jest preset (
jest.preset.js) that provides all necessary configuration including:
- Test matching patterns
- Coverage collection rules
- Module extensions
- Custom resolver
- Module name mapping for special cases
The empty configuration allows the container to inherit these base settings while remaining flexible for different test scenarios.
The setup is consistent with the project's testing architecture, which uses multiple Jest setup files for specific testing requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing Jest configurations in the codebase rg -l "jest.config"Length of output: 67999
Script:
#!/bin/bash # Check content of Jest configuration files rg -l "jest.config" | xargs -I {} sh -c 'echo "=== {} ==="; cat {} 2>/dev/null | grep -A 10 "module.exports"'Length of output: 83118
Script:
#!/bin/bash # Check Jest configuration in package.json and nx.json echo "=== package.json Jest config ===" cat package.json | jq '.jest' echo -e "\n=== nx.json Jest config ===" cat nx.json | jq '.jest'Length of output: 210
Script:
#!/bin/bash # Find and check content of Jest config files echo "=== Searching for Jest config files ===" fd "jest.config" --type f --exec cat {} echo -e "\n=== Checking test setup in package.json ===" cat package.json | jq '.scripts | select(.test)'Length of output: 214994
Script:
#!/bin/bash # Check content of main Jest configuration files echo "=== jest.config.js ===" cat jest.config.js 2>/dev/null || echo "File not found" echo -e "\n=== jest.preset.js ===" cat jest.preset.js 2>/dev/null || echo "File not found" echo -e "\n=== Searching for Jest setup files ===" fd "jest.setup" --type f --exec cat {}Length of output: 2626
Fix lints for our Dockerfiles, with minimal functional change.
Summary by CodeRabbit
New Features
Improvements
ADDcommands withCOPYfor clearer file handling.Style