-
-
Notifications
You must be signed in to change notification settings - Fork 8
added bind options for both service port and metrics port for spacedsust, slingshot, and ufos #42
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
base: main
Are you sure you want to change the base?
Conversation
…st, slingshot, and ufso
| ServerBuilder::new(api, context, log) | ||
| .config(ConfigDropshot { | ||
| bind_address: "0.0.0.0:9999".parse().unwrap(), | ||
| bind_address: bind, |
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.
yessss. i love when fixing hacks actually makes code simpler and safer
|
@jvalinsky thank you!!!! to fix that clippy report you can throw one of these on it #[allow(clippy::too_many_arguments)]
pub async fn serve(
cache: HybridCache<String, CachedRecord>,
...eventually it should be refactored to take a config struct or something, but i don't want to spend time on that now because i kind of want to refactor some of the top-level app-running boilerplate into some helpers across the whole repo, which would probably account for that. silencing clippy here is fine :) a few more fmt things are going to bite you before this passes, but that's the easy part. if you run |
| .await | ||
| } else { | ||
| run(TcpListener::bind("127.0.0.1:3000"), app, shutdown).await | ||
| run(TcpListener::bind(bind.to_string()), app, shutdown).await |
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.
does doing just TcpListener::bind(bind) (without .to_string()) work here?
| host[1], | ||
| host[2], | ||
| host[3] | ||
| "metrics server installed! listening on 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.
clippy is probably yelling at you for not doing
log::info!("metrics server installed! listening on http://{bind_metrics}");here
| host[2], | ||
| host[3] | ||
| "metrics server installed! listening on http://{}", | ||
| bind_metrics |
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.
...and here
uniphil
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.
couple small nits but this is the exact right approach :)
The clippy linting has a rule for too_many_arguments set to a max of 7, my refactored serve func for slingshot has 8 args:
How do you recommend fixing this?