-
Notifications
You must be signed in to change notification settings - Fork 5
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
Config file #162
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 |
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 Support for multiline parameter definitions. Basic type annotations. #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://63ae106793010d836c74830fa75b300c@o264756.ingest.sentry.io/4506186624335872" | ||
ontology_path = "https://odahub.io/ontology/ontology.ttl" |
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.