-
Notifications
You must be signed in to change notification settings - Fork 5
Config file #162
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
base: master
Are you sure you want to change the base?
Config file #162
Conversation
|
With this PR the following changes are introduced:
The config values for the init of the |
dsavchenko
left a comment
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.
The config file would be useful also to
- set the Sentry URL (currently via env var),
- set ontology path (it's used in deploy now, but will be needed in context of #155 to get parameter type from annotation)
I now extract the path from the toml file for the |
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.
Some suggestions on config structure.
Also, it would be good to have an ability to set the path to the config file in cli commands.
As before, I suggest using --settings for file path and rename the arg used to set config options directly from cli (and add it to other cli then). Alternatively, --settings_path is aalso ok
settings.toml
Outdated
| max_download_size = 1000000000 | ||
| n_download_max_tries = 10 | ||
| download_retry_sleep = 0.5 | ||
| sentry_url = "https://[email protected]/4506186624335872" | ||
| ontology_path = "https://odahub.io/ontology/ontology.ttl" No newline at end of file |
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.
These configs belong not only to service module. Probably better to have some separate blocks, e.g. [global] and [service] ?
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.
so, basically only host an port are specific for the service?
settings.toml
Outdated
| @@ -0,0 +1,8 @@ | |||
| [default.service] | |||
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.
Why "default" in the block name? Does it have any special meaning?
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.
It is needed when using the FlaskDynaConf, extension of DynaConf specific for Flask
https://www.dynaconf.com/flask/#settings-files
It needs to have an environment associated to that configuration, and default is the default
This would be mainly used for the service, correct? |
Mainly used: yes. But we need the ability to set the path to the config file in the whatever module that uses this config |
|
The number of changes to be introduced with the config file are far more than what I expected, and requires some more discussions. I am going to split this PR in the two, where the first only tackles the "max size download" matter. |
|
@burnout87 it's still waiting in requested changes. Do you want to complete this? If it is still needed? |
It requires a number of changes that should perhaps be discussed. Also, it'll take some time, so it has to be decided how much priority this actually has. |
No description provided.