-
Notifications
You must be signed in to change notification settings - Fork 20
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
lava: use new dotted-key syntax for storage settings section #293
Conversation
Update how the name of the storage settings section is defined by following the TOML dotted-key syntax with sub-tables. Signed-off-by: Guillaume Tucker <[email protected]>
0f489dd
to
b18c754
Compare
@@ -29,8 +29,7 @@ def _get_api_helper(api_config_name, api_token): | |||
|
|||
def _get_storage(storage_config_name): | |||
storage_config = CONFIGS['storage_configs'][storage_config_name] | |||
section = SETTINGS[(':'.join(('storage', storage_config_name)))] | |||
storage_cred = section['storage_cred'] | |||
storage_cred = SETTINGS['storage'][storage_config_name]['storage_cred'] |
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.
Can we use parse_opts
instead to read the settings from TOML like other services?
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.
No because this is not a service like the other ones, it's a Flask application. There might be things we can do to try to make it look like a service but I made the design decision to rely directly on the YAML config and TOML settings as it seemed like the best choice in this case.
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.
Yes, I am aware it's a Flask app but I was wondering if we could use kernelci
package somehow to read settings. But it seems not. Looks fine otherwise.
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.
Yeah it's just not very practical to run a Flask service which already has its own "main" function or WSGI connector as a kernelci-core command line tool. Also it's very straightforward to just read the settings and YAML config, I think a real improvement would be to help separate that logic from the helpers to run command line tools in kernelci-core. But that's really not a blocking issue at the moment, probably something to revisit once the old tools have been completely deprecated.
Update how the name of the storage settings section is defined by following the TOML dotted-key syntax with sub-tables.