Add config.conf and logic to handle config files #664
Conversation
|
I think we need to think about the path of that file. Where should it be? Should we consider multiple possibilities? like |
|
@rjodinchr I think you are right either we should make a file under /etc/something for system specific configs perhaps? and check the current directory as well. Maybe we can populate a file based on customized configs based on the platform? I am not quite sure myself. WDYT? |
There was a problem hiding this comment.
Thanks for this contribution!
I'm wondering whether we should also add the ability to select a config file from the environment using a variable (e.g. CLVK_CONFIG_FILE=/path/to/clvk.conf.
We also need to think through how we could test the ability to load config files automatically. Testing all cases would be hard but I think we should at least have some testing.
Rekt3421
changed the title
|
have a look at https://github.com/kpet/clvk/blob/main/src/log.hpp, and then change all the prints please. |
There was a problem hiding this comment.
Nice work! This is missing a few finishing touches and documentation. Ideally we'd rework the section on environment variables into a more generic description of the config system, explaining that there is support for files and environment variables. If you're unsure of how to go about it, then just document CLVK_CONFIG_FILE and we can do the rest later.
I'm sure we will need to spend more time on config file path management and system integration but this PR is good enough as a first step IMO.
| * `CLVK_CONFIG_FILE` specifies the path to a configuration file. You can see a | ||
| sample of the file under test/config/conf_test.conf. The file specified by env | ||
| var takes precedence over all other config files but does not take precedence | ||
| over env variable defined values. |
| config_file_paths.push_back( | ||
| (std::filesystem::current_path() / conf_file).string()); | ||
| // First check if env var has file | ||
| std::string conv_file_env_var = "CLVK_CONFIG_FILE"; |
There was a problem hiding this comment.
Why did you introduce a function to compose the name of environment variables used for configuration without using it?
|
|
||
| for (auto& opt : gConfigOptions) { | ||
| if (file_config_values.find(opt.name) == file_config_values.end() || | ||
| file_config_values[opt.name].length() == 0) { |
There was a problem hiding this comment.
I think we don't need to check that the length is zero since option values only get stored in the map if they are not empty strings (see read_config_file).
| @@ -90,8 +199,9 @@ void parse_env() { | |||
|
|
|||
| } // namespace | |||
|
|
|||
| void init_config_from_env_only() { parse_env(); } | |||
There was a problem hiding this comment.
Why are we now parsing the environment config twice?
There was a problem hiding this comment.
Adds configs from .conf file, I decided to go against using individual sections to append the name further since we can do so by keeping everything in a unordered map. Env vars are given priority. Addresses issue #631