-
Notifications
You must be signed in to change notification settings - Fork 185
fix(ci): use official lotus image for devnet check #6320
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
Conversation
WalkthroughThe changes update environment variable naming conventions across Docker Compose services (removing underscores from GOLDEN_WEEK_HEIGHT), switch the Lotus container image to a Filecoin all-in-one variant, modify the API bump workflow to use hardcoded tag formats, and replace the lotus_node healthcheck from curl-based JSON-RPC to shell-based lotus CLI commands. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (4)
Comment |
7411419 to
4a60199
Compare
4a60199 to
23f9017
Compare
|
@hanabi1224 what do we do with this draft? |
|
I will need to investigate the failure |
|
@hanabi1224 as far as I remember, I'm not using an official lotus image because it doesn't contain some binaries; I don't remember which ones (lotus-seed maybe?) |
|
@LesnyRumcajs |
| test: >- | ||
| curl -s -x post -h "content-type: application/json" | ||
| --data '{ "jsonrpc": "2.0", "method": "filecoin.chainhead", "params": [], "id": 1 }' | ||
| http://lotus_node:${LOTUS_RPC_PORT}/rpc/v0 || exit 1 |
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.
The DNS name should be lotus instead of lotus_node. Not sure how this worked : )
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.
This doesn't seem correct. DNS name should be taken from the service name (lotus_node or overridden by hostname). Let me double check.
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.
https://github.com/ChainSafe/forest/blob/main/scripts/devnet/lotus.env#L4
LOTUS_API_LISTENADDRESS=/dns/lotus/tcp/1234/http
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.
ah no, container_name also overrides it. All good.
| - LOTUS_TEEP_HEIGHT=${TEEP_HEIGHT} | ||
| - LOTUS_TOCK_HEIGHT=${TOCK_HEIGHT} | ||
| - LOTUS_GOLDEN_WEEK_HEIGHT=${GOLDEN_WEEK_HEIGHT} | ||
| - LOTUS_GOLDENWEEK_HEIGHT=${GOLDEN_WEEK_HEIGHT} |
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.
4feb544 to
f9f4b53
Compare
|
@LesnyRumcajs could you remind me why we needed a custom Lotus image for network upgrade? |
I think it was either because of missing binaries and/or lack of 2k network image. Probably the latter, which was resolved in filecoin-project/lotus#12911. That said, we still should keep our Lotus Dockerfile and workflows in case we need to work on a specific Lotus commit with some patches. |
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.