Skip to content
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

chore(health): extract health into separate crate #1008

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bragov4ik
Copy link
Contributor

Extract common logic for setting up health endpoint compilation (+ proto file). This way the implementation details are hidden under the functions and can be reused for other crates.

@bragov4ik bragov4ik marked this pull request as ready for review August 16, 2024 08:50
fn main() -> Result<(), Box<dyn std::error::Error>> {
let swagger_crate_folder = Path::new("../../libs/endpoints/health");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swagger or health ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, will this work in other than blockscou-rs repositories and will this build inside docker containers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is better to leave the proto/health/v1/health.proto file as it is used by some of other services

Comment on lines 20 to 23
.field_attribute(
".blockscout.stats.v1.HealthCheckRequest.service",
"#[serde(default)]"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets move that definition into the health endpoint crate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just add optional to service field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to add to proto then remove

use prost_build::Config;

pub fn add_to_compile_config(config: &mut Config) {
config.bytes([".grpc.health"]).type_attribute(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion it is the client code responsibility to decide should the generated code use bytes or not, so I suggest to remove it from here and only leave .bytes(["."]) in the client code

pub fn add_to_compile_config(config: &mut Config) {
config.bytes([".grpc.health"]).type_attribute(
".grpc.health",
"#[actix_prost_macros::serde(rename_all=\"snake_case\")]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, wdyt about leaving the case definition as the client responsibility as well?

fn main() -> Result<(), Box<dyn std::error::Error>> {
let swagger_crate_folder = Path::new("../../libs/endpoints/health");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, will this work in other than blockscou-rs repositories and will this build inside docker containers?

);
}

pub fn proto_files(path_to_swagger_crate: &Path) -> Vec<PathBuf> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn proto_files(path_to_swagger_crate: &Path) -> Vec<PathBuf> {
pub fn proto_files(path_to_health_crate: &Path) -> Vec<PathBuf> {

@bragov4ik
Copy link
Contributor Author

bragov4ik commented Aug 23, 2024

need to fix troubles with versioning/path resolution
(maybe macro 🤔 )

Copy link

@implausibleDeniability implausibleDeniability left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, good job 👍👍👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants