-
Notifications
You must be signed in to change notification settings - Fork 1
Add health endpoint #77
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
dmidem
left a comment
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.
Looks good overall, I just have a few suggestions - mainly around replacing some instances of .unwrap() with .expect("...") .
zebrad/src/components/health.rs
Outdated
|
|
||
| async fn handle_request(req: Request<Incoming>) -> Result<Response<String>, Infallible> { | ||
| // Check if the request is for the health endpoint | ||
| if req.uri().path() != "/health" { |
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.
Should it additionally validate that it uses GET HTTP method only? Tracing endpoint validates that: https://github.com/QED-it/zebra/blob/zsa1/zebrad/src/components/tracing/endpoint.rs#L131
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.
A couple of additional remarks are below.
dmidem
left a comment
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.
CI fails now, to fix it, I suggest the following changes in Cargo.toml.
| insta = { version = "1.40.0", features = ["json"] } | ||
|
|
||
| # zebra-rpc needs the preserve_order feature, it also makes test results more stable | ||
| serde_json = { version = "1.0.132", features = ["preserve_order"] } |
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.
Consider restoring serde_json under [dev-dependencies] (while keeping it in [dependencies] with optional = true), because cargo test without --features=health-endpoint currently fails with unresolved imports in tests that use serde_json directly, e.g.:
error[E0432]: unresolved import `serde_json`
--> zebrad/tests/acceptance.rs:158:5
|
158 | use serde_json::Value;
| ^^^^^^^^^^ help: a similar path exists: `jsonrpc_core::serde_json`
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.
V
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
Co-authored-by: Dmitry Demin <[email protected]>
|
Closed in favor of |
Add health endpoint for Zebra node