-
Notifications
You must be signed in to change notification settings - Fork 24
Add CLI args and env var support #98
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
|
👋 Thanks for assigning @benthecarman as a reviewer! |
cc9d26c to
4d0569f
Compare
|
🔔 1st Reminder Hey @benthecarman! This PR has been waiting for your review. |
benthecarman
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.
needs rebase
Adds support for configuring the node via CLI arguments and environment variables, allowing runtime overrides of the configuration file. - Added `clap` dependency for argument parsing. - Implemented layered config loading: config file (full set of options) + environment variables + CLI arguments. Env vars and CLI args override values from the config file when present. - Implemented `ConfigBuilder` to handle partial state and merging. - Added comprehensive unit tests for precedence and validation logic. - Updated README with usage instructions and explanation of config precedence. Co-authored-by: moisesPomilio <93723302+moisesPompilio@users.noreply.github.com>
| cargo run --bin ldk-server | ||
| ``` | ||
|
|
||
| - Using CLI arguments (all optional): |
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 is calling the cli, not running the server with command line args
| } | ||
|
|
||
| #[derive(Debug)] | ||
| #[derive(Debug, PartialEq)] |
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.
nit: can also do Eq
| #[cfg(any(feature = "events-rabbitmq", feature = "experimental-lsps2-support"))] | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| format!( | ||
| "To use the `{}` feature, you must provide a configuration file.", | ||
| if cfg!(feature = "events-rabbitmq") { | ||
| "events-rabbitmq" | ||
| } else { | ||
| "experimental-lsps2-support" | ||
| } | ||
| ), | ||
| )); | ||
| } |
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.
Why is this the else case? If no config file is specified in the args, we should instead look for one in the default data dir
| Ok(config) => config, | ||
| Err(e) => { | ||
| eprintln!("Invalid configuration file: {}", e); | ||
| eprintln!("Invalid configuration: {}", e); |
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.
| eprintln!("Invalid configuration: {}", e); | |
| eprintln!("Invalid configuration: {e}"); |
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.
small preference
|
|
||
| let mut ldk_node_config = Config::default(); | ||
| let config_file = match load_config(&config_path) { | ||
| let config_file = match load_config(&args_config) { |
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.
Config path and the args above is never used now. We should be able to remove that logic as we use clap now, but we still should check for the default config file
Continues the work started in #69 by @moisesPompilio adding support for configuring the node via CLI arguments and environment variables, allowing runtime overrides of the configuration file.
I introduced
ConfigBuilderto handle partial state and merging of the configuration fields, which significantly cleaned up theload_configlogic.