config: allow user-config #1318
Conversation
Allow a user to maintain a config file in their home directory, which overrides the default settings. This makes it possible to execute the test suite from a clean git repo state and reduces the probability that config changes are accidentally committed to the repo. Signed-off-by: Moritz Röhrich <[email protected]>
harvester_e2e_tests/conftest.py
Outdated
| with open('config.yml') as f: | ||
| config_data = yaml.safe_load(f) | ||
| user_home = os.getenv('HOME') | ||
| user_config_path= os.path.join(user_home, '.config', 'harvester', 'tests', |
There was a problem hiding this comment.
IMO it's better to use an env. vairable like HVST_TESTS_CONFIG_PATH to give user the flexibility instead of fixed path.
e.g. user_config_path = os.getenv('HVST_TESTS_CONFIG_PATH')
There was a problem hiding this comment.
Good point. I've added the variable HARVESTER_TESTS_CONFIG as possible location for the config file.
I still want it to look in a default location outside of the repository, since this way dropping a file somewhere is enough and the user doesn't need to set an environment variable as well.
I've also put a short line in the readme to explain all this.
| for key in user_data: | ||
| if not key in default_data.keys(): | ||
| config[key] = user_data[key] | ||
|
|
There was a problem hiding this comment.
May be more pythonic?
>>> dcnf = {'A': 1, 'B': 2}
>>> ucnf = {'a': 10, 'B': 20, 'c': 30}
>>> ucnf | dcnf # python 3.9+
{'a': 10, 'B': 2, 'c': 30, 'A': 1}
>>>
>>> {**ucnf, **dcnf} # python 3.5+
{'a': 10, 'B': 2, 'c': 30, 'A': 1}
There was a problem hiding this comment.
Thanks for pointing this out. Yes, it would be better to use a more pythoninc way to express the dict update.
I opted to use the dict.update method, since this allows me to loop over a list of potential config sources and reuse the file-reading logic.
It also makes it very clear what's happening and doesn't depend on a python version.
Read user config path from environment variable `HARVESTER_TESTS_CONFIG` Simplified config data loading. Signed-off-by: Moritz Röhrich <[email protected]>
There was a problem hiding this comment.
make it as a OS environment variable is not a good idea, it is not explicitly point out where the configuration comes from.
we can simply add new configure option --config and let the default value be config.yml would gracefully resolve the config file name problem.
for me, I will not accept any OS environment or implicit variables injection, because we are not making a tool, it is for test, everything should be explicitly.
| @@ -421,3 +449,18 @@ def pytest_html_results_table_header(cells): | |||
|
|
|||
| def pytest_html_results_table_row(report, cells): | |||
| cells.insert(1, f'<td class="col-time">{datetime.utcnow()}</td>') | |||
|
|
|||
|
|
|||
| def pytest_report_header(config): | |||
There was a problem hiding this comment.
I don't think it is a good idea to expose sensitive data, it is not necessary for the report.
Agree, it's more explicit than env. variable approach. |
This is not simple at all. The way it currently works is that we load the config data first and then populate the default values for the command line options with the data loaded from the config file. Having a command line option that defines which file to load the config data from would mean the that we would need to update the final |
we would not need other command line options, to let the config filename configurable, we will remove other options, they would only be update in the config file. |
Allow a user to maintain a config file in their home directory, which overrides the default settings.
This makes it possible to execute the test suite from a clean git repo state and reduces the probability that config changes are accidentally committed to the repo.