Skip to content

Conversation

@Anyitechs
Copy link
Contributor

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 ConfigBuilder to handle partial state and merging of the configuration fields, which significantly cleaned up the load_config logic.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 16, 2026

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs Anyitechs force-pushed the config-env branch 2 times, most recently from cc9d26c to 4d0569f Compare January 16, 2026 16:25
@Anyitechs Anyitechs marked this pull request as ready for review January 16, 2026 16:46
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Collaborator

@benthecarman benthecarman left a 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):
Copy link
Collaborator

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)]
Copy link
Collaborator

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

Comment on lines +509 to +521
#[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"
}
),
));
}
Copy link
Collaborator

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Invalid configuration: {}", e);
eprintln!("Invalid configuration: {e}");

Copy link
Collaborator

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) {
Copy link
Collaborator

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

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.

3 participants