fix: broken start-erigon make target #2186
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes made in the pull request primarily involve updates to the Changes
Assessment against linked issues
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2186 +/- ##
==========================================
+ Coverage 27.47% 27.64% +0.17%
==========================================
Files 366 366
Lines 16662 16717 +55
Branches 12 12
==========================================
+ Hits 4578 4622 +44
- Misses 11777 11786 +9
- Partials 307 309 +2 |
However, its still not solved as erigon does not respect the --networkid which causes beacond to fail since its expecting chainid 80087. It seems this issue was fixed in erigontech/erigon#9408 but even with pulling a recent docker image its still not working
fridrik01
force-pushed
the
fix-start-erigon-maketarget
branch
from
9e69e5f to
829de29
Compare
fridrik01
changed the title
![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
🧹 Outside diff range comments (1)
build/scripts/testing.mk (1)
JWT file permissions need adjustment for container user
The JWT file currently has permissions
644(-rw-r--r--) and is owned byjailuser, which may cause issues when running the container with UID 1000. To ensure proper operation, the JWT file should be readable by the container user.
testing/files/jwt.hex: Change ownership to UID 1000 or ensure the file has world-readable permissionsExample fix:
chmod 644 testing/files/jwt.hex chown 1000:1000 testing/files/jwt.hex🔗 Analysis chain
Line range hint
214-230: LGTM! Verify JWT file permissions.The changes maintain consistency with the init phase and correctly handle paths within the container context. The JWT secret and data directory paths are properly aligned with their respective volume mounts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify JWT file permissions # The JWT file should be readable by UID 1000 # Find JWT file jwt_file=$(find . -name "jwt.hex") if [ -n "$jwt_file" ]; then stat -c "JWT file permissions: %a Owner: %u:%g" "$jwt_file" # Check if user 1000 can read the file runuser -u \#1000 -- test -r "$jwt_file" && echo "UID 1000 can read JWT file" || echo "Warning: UID 1000 cannot read JWT file" fiLength of output: 506
Script:
#!/bin/bash # Let's try with ls command which is more commonly available jwt_file=$(find . -name "jwt.hex") if [ -n "$jwt_file" ]; then ls -l "$jwt_file" fiLength of output: 198
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
build/scripts/testing.mk(2 hunks)
🔇 Additional comments (1)
build/scripts/testing.mk (1)
203-208: LGTM! Verify user ID matches host system.
The changes correctly address the path resolution issues and user permissions by:
- Running as user 1000:1000 to match typical host user permissions
- Using absolute paths within the container context
✅ Verification successful
The user ID configuration is appropriate for Docker container isolation
The changes to use --user 1000:1000 are correct and safe because:
- It's a standard practice to use UID 1000 for Docker containers as it provides good isolation
- The container runs in an ephemeral mode (
--rmflag) and only mounts specific volumes - File permissions are scoped to the mounted volumes only
The inability to verify the host system's UID is not a concern since:
- The container runs in isolation
- The mounted volumes are relative to the project directory
- It's an ephemeral testing environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that UID 1000 exists on the host system
# This helps ensure the Docker container's user permissions will work correctly
id 1000 || echo "Warning: UID 1000 does not exist on host system"
Length of output: 207
Script:
#!/bin/bash
# Let's check if the Dockerfile for erigon is available in the repository
# to verify the user configuration
fd Dockerfile | grep -i erigon
Length of output: 32
Script:
#!/bin/bash
# Let's also check the make target definition to understand the full context
rg "start-erigon:" -A 10
Length of output: 602
Script:
#!/bin/bash
# Search for any documentation or configuration files related to erigon setup
fd -e md -e txt -e conf | grep -i erigon
Length of output: 42
fridrik01
changed the title
|
Tested and it's working 🎉 |
Fixes: #2169
This PR fixes the
start-erigonmake target. The root cause was invalid paths used as params to docker and that erigon Dockerfile had changed to a different userid/group. To address that I specified to docker to run as user 1000:1000 as it was before it was changed (now it defaults to having a erigon user with id 3473:3473)Summary by CodeRabbit