Repository navigation
chore: fix jwt secret flag - #150
Conversation
Update evstack health checks to use the new HTTP /health/ready endpoint instead of the gRPC /evnode.v1.HealthService/Livez endpoint. Changes: - Replace gRPC endpoint with /health/ready HTTP endpoint - Update health check method from POST to GET - Continue using RPC port (7331) where HTTP endpoints are served - Remove unused bytes import
…ix-jwt-secret-flag
WalkthroughThree Docker/EVM node configuration files updated: health check endpoints switched from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
framework/docker/evstack/evmsingle/node.go (1)
208-238: LGTM! Improved readiness check implementation.The readiness check has been enhanced with several improvements:
- Context cancellation is properly respected (lines 216-220)
- TCP dial check before HTTP request reduces spurious errors (lines 222-227)
- Health endpoint updated to REST-style
/health/readyGET request (lines 211, 228)These changes make the readiness check more robust and align with the health check updates across the codebase.
Minor note: Line 228 ignores the error from
http.NewRequestWithContextwith_. While this shouldn't fail with valid parameters, it's technically more defensive to check the error. This is optional and not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
framework/docker/evmsingle_with_reth_test.go(1 hunks)framework/docker/evstack/evmsingle/node.go(5 hunks)framework/docker/evstack/node.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: chatton
Repo: celestiaorg/tastora PR: 106
File: framework/docker/dataavailability/node.go:259-268
Timestamp: 2025-08-27T08:29:14.500Z
Learning: The user chatton prefers to keep existing logic unchanged during refactors, even when there are potential improvements, to maintain focus on the structural changes and avoid introducing bugs during code movement.
📚 Learning: 2025-08-27T08:24:51.731Z
Learnt from: chatton
Repo: celestiaorg/tastora PR: 106
File: framework/docker/dataavailability/config.go:3-7
Timestamp: 2025-08-27T08:24:51.731Z
Learning: The tastora codebase consistently uses `github.com/moby/moby/client` for Docker client imports throughout all framework/docker packages. Do not suggest changing this to `github.com/docker/docker/client` as it would break type compatibility.
Applied to files:
framework/docker/evmsingle_with_reth_test.go
🧬 Code graph analysis (3)
framework/docker/evmsingle_with_reth_test.go (1)
framework/types/network.go (1)
Ports(50-63)
framework/docker/evstack/node.go (3)
framework/docker/evstack/evmsingle/node.go (1)
Node(21-32)framework/docker/evstack/reth/node.go (1)
Node(23-31)framework/docker/dataavailability/node.go (1)
Node(61-77)
framework/docker/evstack/evmsingle/node.go (1)
framework/docker/evstack/node.go (1)
Node(40-55)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
framework/docker/evmsingle_with_reth_test.go (1)
191-193: LGTM! Health check endpoint updated correctly.The migration from the gRPC-style
/evnode.v1.HealthService/LivezPOST endpoint to the simpler REST/health/readyGET endpoint is clean and consistent with the changes across other files in this PR.framework/docker/evstack/node.go (1)
260-260: LGTM! Consistent health check migration.The health endpoint has been successfully updated to use
/health/readywith a GET request, and the comment accurately reflects the new ready-check behavior. This aligns well with the changes in the test file and the broader PR objectives.Also applies to: 282-284
framework/docker/evstack/evmsingle/node.go (5)
8-8: Good addition ofpathimport.The
pathpackage is appropriate for constructing Unix paths in Docker containers, as used in line 164 for the JWT secret file path.
129-130: LGTM! Flag renaming from rollkit to evnode.The aggregator and signer flags have been correctly updated to use the
evnodeprefix, consistent with the broader namespace migration mentioned in the PR objectives.
156-160: LGTM! JWT secret file writing implemented correctly.The JWT secret is now properly written to a file as per the PR objectives. The error handling is correct, and the file-based approach is more secure than passing secrets directly as command-line arguments.
Note: The JWT secret file is placed in the home directory root while the passphrase file (line 124) is in the
config/subdirectory. This organizational difference is acceptable but could be standardized in future refactoring if desired.
164-164: LGTM! JWT secret flag correctly updated.The flag has been properly changed from
--evm.jwt-secretto--evm.jwt-secret-file, with the file path correctly constructed usingpath.Join(n.HomeDir(), "jwt-secret.txt"), matching where the file was written on line 158.
175-175: LGTM! Consistent flag renaming in start command.The evnode flags in the start command match the naming used in the init command (lines 129-130), ensuring consistency across the node lifecycle.
Overview
Fixes the evm-single type to write the contents of the jwt token as a file instead of passing as a field, pulls in the changes made in #149
cc @randygrok
closes #149
Summary by CodeRabbit