chore: dev dependencies removed in docker build #1742
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe pull request introduces a new shell script Changes
Sequence DiagramsequenceDiagram
participant Dockerfile
participant Script as adjust-packages.sh
participant PackageJSON as package.json files
Dockerfile->>Script: Copy and execute script
Script->>PackageJSON: Find package.json files
Script->>PackageJSON: Remove devDependencies
Script->>PackageJSON: Simplify dependency versions
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
Test CoverageSummary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docker/rpc-proxy/adjust-packages.sh (2)
1-10: Enhance script documentation.Consider adding the following details to the documentation:
- Prerequisites (e.g., jq dependency)
- Exit codes and their meanings
- Error handling behavior
#!/bin/bash # ----------------------------------------------------------------------------- # Info: +# Prerequisites: +# - jq: JSON processor (required) +# # - input: directory to search for package.json files # - output: # package.json without devDependencies section # package.json without version selectors in dependencies +# +# Exit codes: +# 0: Success +# 1: Invalid/missing input directory +# 2: jq command not found # -----------------------------------------------------------------------------
42-42: Improve version selector handling.The current version selector removal logic might miss some cases:
- Only handles caret (^) and tilde (~)
- Multiple ltrimstr might not handle all cases correctly
Consider using a more robust approach:
- jq '(.dependencies |= with_entries(.value |= ltrimstr("^") | ltrimstr("~")))' "$PACKAGE_JSON" > temp.json && mv temp.json "$PACKAGE_JSON" + jq '(.dependencies |= with_entries(.value |= sub("^[~^]"; "") | sub("^>=?"; "") | sub("^<=?"; "") | sub("^="; "")))' "$PACKAGE_JSON" > "$TEMP_FILE" && mv -f "$TEMP_FILE" "$PACKAGE_JSON"docker/rpc-proxy/Dockerfile (1)
22-25: Optimize the package cleaning steps.While the implementation is functionally correct, we can optimize these steps to reduce the number of layers and improve maintainability.
Consider combining the steps into a single RUN instruction:
-# Clean the package.json files ready for production -RUN apk add --no-cache jq -RUN chmod +x ./adjust-packages.sh -RUN /bin/sh ./adjust-packages.sh ./ +# Clean the package.json files ready for production +RUN apk add --no-cache jq && \ + chmod +x ./adjust-packages.sh && \ + ./adjust-packages.sh ./This change:
- Reduces the number of layers in the final image
- Maintains the same functionality
- Removes the redundant
/bin/shinvocation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker/rpc-proxy/Dockerfile(1 hunks)docker/rpc-proxy/adjust-packages.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: rpc-proxy / docker / docker
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
docker/rpc-proxy/adjust-packages.sh (1)
1-48: Verify Docker build integration.Let's ensure the script is properly integrated into the Docker build process and that it effectively removes development dependencies.
✅ Verification successful
Docker integration verified successfully ✓
The script is properly integrated into the Docker build process with all necessary components in place:
- Installed dependencies (jq)
- Correct permissions
- Executed at the right stage (after build, before production copy)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify script integration in Dockerfile and its effectiveness # Test 1: Check if the script is referenced in Dockerfile echo "Checking Dockerfile integration..." rg -l "adjust-packages.sh" --type dockerfile # Test 2: Verify script permissions in Dockerfile echo "Checking script permissions..." rg "chmod.*adjust-packages.sh" --type dockerfile # Test 3: Check if jq is installed in Dockerfile echo "Checking jq installation..." rg "jq" --type dockerfileLength of output: 441
Script:
#!/bin/bash # Find and examine Dockerfile contents echo "=== Locating Dockerfiles ===" fd -t f "^[Dd]ockerfile" echo -e "\n=== Examining Dockerfile contents ===" fd -t f "^[Dd]ockerfile" -x cat {}Length of output: 3175
docker/rpc-proxy/Dockerfile (2)
17-17: Verify the contents of adjust-packages.sh script.The placement of the COPY instruction is correct. However, we should verify the script's implementation to ensure it correctly handles package.json modifications.
Let's examine the script's contents:
✅ Verification successful
Script implementation is correct and safe for package.json processing
The script correctly:
- Removes development dependencies from package.json files
- Strips version selectors for stable builds
- Includes proper error handling and atomic updates
- Has appropriate executable permissions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents and permissions of adjust-packages.sh # Check if the script exists and show its contents cat ./docker/rpc-proxy/adjust-packages.sh # Verify script permissions ls -l ./docker/rpc-proxy/adjust-packages.shLength of output: 1950
Line range hint
1-56: Verify build process after dependency cleanup.The implementation correctly addresses the PR objective of removing development dependencies. However, we should verify that:
- The build process still works after removing devDependencies
- Workspace dependencies are correctly handled
Let's verify the build process:
✅ Verification successful
Dependencies and build process are properly configured
The implementation correctly:
- Maintains all required runtime dependencies
- Uses proper workspace package structure
- Implements clean separation between build and runtime stages
- Handles production dependency installation correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the build process and workspace dependencies # Check workspace dependencies in package.json files echo "Checking workspace dependencies..." find . -name "package.json" -exec jq '.workspaces, .dependencies' {} \; # List all dependencies that will be installed in production echo "Listing production dependencies..." yarn workspace @vechain/sdk-rpc-proxy list --production --depth=0Length of output: 3456
Script:
#!/bin/bash # Description: Verify production dependencies using npm instead of yarn # Check the rpc-proxy package.json directly echo "Checking RPC proxy package.json..." cat packages/rpc-proxy/package.json | jq '{name: .name, dependencies: .dependencies, peerDependencies: .peerDependencies}' # List files that will be included in the production build echo -e "\nChecking production build files..." find packages/rpc-proxy/dist -type f 2>/dev/null || echo "dist directory not found (build not executed)"Length of output: 767
Comment on lines
+11
to
+22
| # Check if the search directory is provided | ||
| if [ -z "$1" ]; then | ||
| echo "Usage: $0 <path-to-directory>" | ||
| exit 1 | ||
| fi | ||
| SEARCH_DIR="$1" | ||
|
|
||
| # Check if the search directory exists | ||
| if [ ! -d "$SEARCH_DIR" ]; then | ||
| echo "Error: Directory '$SEARCH_DIR' not found." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential error handling and cleanup.
The script needs additional safeguards:
- Validate jq dependency
- Clean up temporary files on exit
- Handle script interruption
+# Check if jq is available
+if ! command -v jq >/dev/null 2>&1; then
+ echo "Error: jq is required but not installed."
+ exit 2
+fi
+
+# Cleanup function
+cleanup() {
+ rm -f temp.json
+ exit $?
+}
+
+# Set up trap for cleanup
+trap cleanup EXIT
+trap 'exit 2' INT TERM
+
# Check if the search directory is provided
if [ -z "$1" ]; then
echo "Usage: $0 <path-to-directory>"📝 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.
Suggested change
| # Check if the search directory is provided | |
| if [ -z "$1" ]; then | |
| echo "Usage: $0 <path-to-directory>" | |
| exit 1 | |
| fi | |
| SEARCH_DIR="$1" | |
| # Check if the search directory exists | |
| if [ ! -d "$SEARCH_DIR" ]; then | |
| echo "Error: Directory '$SEARCH_DIR' not found." | |
| exit 1 | |
| fi | |
| # Check if jq is available | |
| if ! command -v jq >/dev/null 2>&1; then | |
| echo "Error: jq is required but not installed." | |
| exit 2 | |
| fi | |
| # Cleanup function | |
| cleanup() { | |
| rm -f temp.json | |
| exit $? | |
| } | |
| # Set up trap for cleanup | |
| trap cleanup EXIT | |
| trap 'exit 2' INT TERM | |
| # Check if the search directory is provided | |
| if [ -z "$1" ]; then | |
| echo "Usage: $0 <path-to-directory>" | |
| exit 1 | |
| fi | |
| SEARCH_DIR="$1" | |
| # Check if the search directory exists | |
| if [ ! -d "$SEARCH_DIR" ]; then | |
| echo "Error: Directory '$SEARCH_DIR' not found." | |
| exit 1 | |
| fi |
Comment on lines
+25
to
+48
| # Find all package.json files in the specified directory (excluding node_modules) | ||
| find "$SEARCH_DIR" -type f -name "package.json" ! -path "*/node_modules/*" | while read PACKAGE_JSON; do | ||
| echo "Processing: $PACKAGE_JSON" | ||
|
|
||
|
|
||
| # Remove the "devDependencies" section and overwrite the file | ||
| if jq -e '.devDependencies' "$PACKAGE_JSON" > /dev/null; then | ||
| jq 'del(.devDependencies)' "$PACKAGE_JSON" > temp.json && mv temp.json "$PACKAGE_JSON" | ||
| echo "devDependencies removed from $PACKAGE_JSON" | ||
| else | ||
| echo "No devDependencies section found in $PACKAGE_JSON. No changes made." | ||
| fi | ||
|
|
||
| # Remove version selectors from dependencies | ||
| # Check if the dependencies section exists in the package.json file | ||
| if jq -e '.dependencies' "$PACKAGE_JSON" > /dev/null; then | ||
| # Remove version selectors from dependencies | ||
| jq '(.dependencies |= with_entries(.value |= ltrimstr("^") | ltrimstr("~")))' "$PACKAGE_JSON" > temp.json && mv temp.json "$PACKAGE_JSON" | ||
| echo "Version selectors (caret/tilde) removed from dependencies in $PACKAGE_JSON" | ||
| else | ||
| echo "No dependencies section found in $PACKAGE_JSON. No changes made." | ||
| fi | ||
|
|
||
| done |
There was a problem hiding this comment.
Fix potential race conditions and improve error handling.
The current implementation has several potential issues:
- Race condition with shared temp.json
- Missing error handling for jq operations
- Potential issues with filenames containing spaces
- Non-atomic file operations
Apply these improvements:
+# Create a unique temporary file
+TEMP_FILE=$(mktemp)
+
# Find all package.json files in the specified directory (excluding node_modules)
-find "$SEARCH_DIR" -type f -name "package.json" ! -path "*/node_modules/*" | while read PACKAGE_JSON; do
+find "$SEARCH_DIR" -type f -name "package.json" ! -path "*/node_modules/*" -print0 | while IFS= read -r -d '' PACKAGE_JSON; do
echo "Processing: $PACKAGE_JSON"
# Remove the "devDependencies" section and overwrite the file
if jq -e '.devDependencies' "$PACKAGE_JSON" > /dev/null; then
- jq 'del(.devDependencies)' "$PACKAGE_JSON" > temp.json && mv temp.json "$PACKAGE_JSON"
+ if ! jq 'del(.devDependencies)' "$PACKAGE_JSON" > "$TEMP_FILE"; then
+ echo "Error: Failed to process devDependencies in $PACKAGE_JSON"
+ continue
+ fi
+ if ! mv -f "$TEMP_FILE" "$PACKAGE_JSON"; then
+ echo "Error: Failed to update $PACKAGE_JSON"
+ continue
+ fi
echo "devDependencies removed from $PACKAGE_JSON"
else
echo "No devDependencies section found in $PACKAGE_JSON. No changes made."
fi
# Remove version selectors from dependencies
# Check if the dependencies section exists in the package.json file
if jq -e '.dependencies' "$PACKAGE_JSON" > /dev/null; then
# Remove version selectors from dependencies
- jq '(.dependencies |= with_entries(.value |= ltrimstr("^") | ltrimstr("~")))' "$PACKAGE_JSON" > temp.json && mv temp.json "$PACKAGE_JSON"
+ if ! jq '(.dependencies |= with_entries(.value |= ltrimstr("^") | ltrimstr("~")))' "$PACKAGE_JSON" > "$TEMP_FILE"; then
+ echo "Error: Failed to process dependencies in $PACKAGE_JSON"
+ continue
+ fi
+ if ! mv -f "$TEMP_FILE" "$PACKAGE_JSON"; then
+ echo "Error: Failed to update $PACKAGE_JSON"
+ continue
+ fi
echo "Version selectors (caret/tilde) removed from dependencies in $PACKAGE_JSON"
else
echo "No dependencies section found in $PACKAGE_JSON. No changes made."
fi
done
+
+# Check if the loop completed successfully
+EXIT_CODE=$?
+if [ $EXIT_CODE -ne 0 ]; then
+ echo "Error: Script failed with exit code $EXIT_CODE"
+ exit $EXIT_CODE
+fi📝 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.
Suggested change
| # Find all package.json files in the specified directory (excluding node_modules) | |
| find "$SEARCH_DIR" -type f -name "package.json" ! -path "*/node_modules/*" | while read PACKAGE_JSON; do | |
| echo "Processing: $PACKAGE_JSON" | |
| # Remove the "devDependencies" section and overwrite the file | |
| if jq -e '.devDependencies' "$PACKAGE_JSON" > /dev/null; then | |
| jq 'del(.devDependencies)' "$PACKAGE_JSON" > temp.json && mv temp.json "$PACKAGE_JSON" | |
| echo "devDependencies removed from $PACKAGE_JSON" | |
| else | |
| echo "No devDependencies section found in $PACKAGE_JSON. No changes made." | |
| fi | |
| # Remove version selectors from dependencies | |
| # Check if the dependencies section exists in the package.json file | |
| if jq -e '.dependencies' "$PACKAGE_JSON" > /dev/null; then | |
| # Remove version selectors from dependencies | |
| jq '(.dependencies |= with_entries(.value |= ltrimstr("^") | ltrimstr("~")))' "$PACKAGE_JSON" > temp.json && mv temp.json "$PACKAGE_JSON" | |
| echo "Version selectors (caret/tilde) removed from dependencies in $PACKAGE_JSON" | |
| else | |
| echo "No dependencies section found in $PACKAGE_JSON. No changes made." | |
| fi | |
| done | |
| # Create a unique temporary file | |
| TEMP_FILE=$(mktemp) | |
| # Find all package.json files in the specified directory (excluding node_modules) | |
| find "$SEARCH_DIR" -type f -name "package.json" ! -path "*/node_modules/*" -print0 | while IFS= read -r -d '' PACKAGE_JSON; do | |
| echo "Processing: $PACKAGE_JSON" | |
| # Remove the "devDependencies" section and overwrite the file | |
| if jq -e '.devDependencies' "$PACKAGE_JSON" > /dev/null; then | |
| if ! jq 'del(.devDependencies)' "$PACKAGE_JSON" > "$TEMP_FILE"; then | |
| echo "Error: Failed to process devDependencies in $PACKAGE_JSON" | |
| continue | |
| fi | |
| if ! mv -f "$TEMP_FILE" "$PACKAGE_JSON"; then | |
| echo "Error: Failed to update $PACKAGE_JSON" | |
| continue | |
| fi | |
| echo "devDependencies removed from $PACKAGE_JSON" | |
| else | |
| echo "No devDependencies section found in $PACKAGE_JSON. No changes made." | |
| fi | |
| # Remove version selectors from dependencies | |
| # Check if the dependencies section exists in the package.json file | |
| if jq -e '.dependencies' "$PACKAGE_JSON" > /dev/null; then | |
| # Remove version selectors from dependencies | |
| if ! jq '(.dependencies |= with_entries(.value |= ltrimstr("^") | ltrimstr("~")))' "$PACKAGE_JSON" > "$TEMP_FILE"; then | |
| echo "Error: Failed to process dependencies in $PACKAGE_JSON" | |
| continue | |
| fi | |
| if ! mv -f "$TEMP_FILE" "$PACKAGE_JSON"; then | |
| echo "Error: Failed to update $PACKAGE_JSON" | |
| continue | |
| fi | |
| echo "Version selectors (caret/tilde) removed from dependencies in $PACKAGE_JSON" | |
| else | |
| echo "No dependencies section found in $PACKAGE_JSON. No changes made." | |
| fi | |
| done | |
| # Check if the loop completed successfully | |
| EXIT_CODE=$? | |
| if [ $EXIT_CODE -ne 0 ]; then | |
| echo "Error: Script failed with exit code $EXIT_CODE" | |
| exit $EXIT_CODE | |
| fi |
freemanzMrojo
approved these changes
Jan 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Remove dev dependencies in RPC proxy docker build
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Summary by CodeRabbit
package.jsonfiles