-
Notifications
You must be signed in to change notification settings - Fork 37
feat(healthz): add version #353
feat(healthz): add version #353
Conversation
b05065a
to
1ef0bcc
Compare
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.
Nice, this looks good to me. Ping @thomastaylor312 for a second review.
Hi @thomastaylor312, could you help me review this PR? I need it to add some warning messages of inconsistent versions in spin#786. Thank you. |
Cargo.toml
Outdated
@@ -27,7 +27,7 @@ maintenance = { status = "actively-developed" } | |||
|
|||
[features] | |||
default = ["server", "client", "caching", "test-tools", "native-tls"] | |||
server = ["warp", "openid", "hyper", "mime", "either", "_common"] | |||
server = ["clap", "warp", "openid", "hyper", "mime", "either", "_common"] |
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.
We should not be bringing in clap
for a server dependency. It is fairly big and we don't want people to import it unless they are actually building the CLI.
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.
Removed it. Thanks!
src/server/routes.rs
Outdated
let health = warp::path("healthz").map(|| { | ||
warp::reply::json(&HealthStatus { | ||
status: "OK".to_string(), | ||
version: clap::crate_version!().to_string(), |
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 can be swapped out to a non-clap thing by doing env!("CARGO_PKG_VERSION")
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.
Updated it. Thank you.
src/server/routes.rs
Outdated
use warp::Filter; | ||
|
||
use crate::{server::filters, signature::KeyRing}; | ||
|
||
#[derive(Deserialize, Serialize)] | ||
pub struct HealthStatus { |
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.
Is this a type that we expect people to read back from the server using a client? If so, would it be better to put into invoice/api.rs
or another location?
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.
Moved it to invoice/api.rs
and also changed the name to HealthResponse
.
Signed-off-by: Frank Yang <[email protected]>
1ef0bcc
to
9347ac3
Compare
resolve #350
ref spinframework/spin#786 (comment)
Test step
/healthz
.