Add file based Config support. #28
Conversation
| args[0] | ||
| ); | ||
| if args.len() < 2 { | ||
| eprintln!("Usage: {} config_path", args[0]); |
There was a problem hiding this comment.
what's the config strategy we wanna pursue? Just passing the file path? A lot of libraries have a multi-layered approach, using a config file, which can be overridden by environment variables, which can be overridden by command-line arguments, and all of that is neatly integrated into man and --help, but typically uses a third-party library.
There was a problem hiding this comment.
I think adding multiple levels of config knobs could add confusion,
for now i chose to keep it simple json config without any config reading dependencies.
With more and more config options, cli args won't really be feasible.
Although we could add support for overriding some config-values with env vars in future if needed.
There was a problem hiding this comment.
Ok, fair enough. What about defaults? It looks like currently the JSON file needs to have all the fields? Also, does the reading of the JSON file currently allow for additional fields that aren't used by us? Or even comment lines?
There was a problem hiding this comment.
Defaults will need to be configured/handled in code.
Yes, currently it needs all the fields to be present, it does allow unknown-fields not being used by us. (added a test)
And doesn't allow comments currently. :(
I did have the option to use something like toml but wasn't sure about crate dependency for it.
There was a problem hiding this comment.
Added basic functionality to remove simple single-line comments using '//', tested in tests in latest commit.
Key/value can't contain '//' without escaping since it is a control character.
|
Pushed fixup commit. |
|
Squashed fixups ! |
2 participants
Allows us to provide config params using ldk-server-config file.