-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix pre-baked images builder #342
Conversation
WalkthroughThe pull request introduces modifications to the Dockerfile for the Foundry environment, specifically changing the Changes
Sequence DiagramsequenceDiagram
participant Dockerfile as Dockerfile
participant Foundry as Foundry Installation
participant Environment as Build Environment
Dockerfile->>Foundry: Install using foundryup -i
Foundry-->>Dockerfile: Complete installation
Dockerfile->>Environment: Set up contract deployment environment
Dockerfile->>Environment: Configure node registration
The sequence diagram illustrates the simplified flow of the Dockerfile, highlighting the Foundry installation process and subsequent environment configuration steps. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dev/baked/Dockerfile (2)
Line range hint
2-2
: Consider using a specific Foundry version instead of nightlyUsing
nightly
as theFOUNDRY_VERSION
could lead to reproducibility issues since nightly builds can change daily. This might affect the deterministic nature of the pre-baked state.Consider:
- Using a specific version number for better reproducibility
- Documenting the version choice in a comment
- Setting up a process to regularly update to newer stable versions
-ARG FOUNDRY_VERSION=nightly +ARG FOUNDRY_VERSION=<specific-version> # Update periodically with stable releases
Line range hint
20-25
: Consider a more robust approach for anvil shutdownThe current approach using
pkill
andsleep
is timing-dependent and could be fragile on different systems or under different loads.Consider implementing a more robust solution:
- Use a proper signal handling mechanism
- Add a health check to verify the state was properly persisted
- Implement retry logic if needed
Example approach:
- pkill -f anvil && \ - sleep 5 + pkill -f anvil && \ + while pgrep -f anvil >/dev/null; do sleep 0.1; done && \ + while [ ! -f anvil-baked-state ]; do sleep 0.1; done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dev/baked/Dockerfile
(1 hunks)
🔇 Additional comments (1)
dev/baked/Dockerfile (1)
17-17
: Verify the foundryup flag change
The change from -v
to -i
aligns with the PR objective to fix the pre-baked images builder. However, let's verify this is the correct new syntax.
For some reason foundryup changed from
-u
to-i
...foundry-rs/foundry#9551
Summary by CodeRabbit