General updates to the library #9
Conversation
|
|
||
|
|
||
| def configure_logging(level_override=None): | ||
| class Config(object): |
There was a problem hiding this comment.
General question: have you removed the ability to configure the app via a JSON file, and now it's only possible to configure via OS environment variables? I personally find JSON config a lot easier to manage. Seems like we could have both if we attempted to load a JSON file from some path(s) before defining this class, then adding a helper method that tries to get the value from the OS environment, then the JSON config file dict if that doesn't exist, then a default value if that doesn't exist.
There was a problem hiding this comment.
I talked about it with Tobias and he agreed on the change
gitreload/config.py
Outdated
| '%(filename)s:%(lineno)d - ' | ||
| '{hostname}- %(message)s').format(hostname=HOSTNAME) | ||
| LOG_FILE_PATH = os.environ.get('LOG_FILE_PATH', '') | ||
| SUBPROCESS_TIMEOUT = int(os.environ.get('SUBPROCESS_TIMEOUT', 60)) * MINUTE |
There was a problem hiding this comment.
Would probably help to call this variable SUBPROCESS_TIMEOUT_MINUTES or something. I typically assume that a timeout value is in seconds
There was a problem hiding this comment.
good point: I'll change that
There was a problem hiding this comment.
I'll change only the environment variable name and keep the config one as is, given that I translate that in seconds.
|
@gsidebo Done |
|
The PR has been merged. How/can we release this @mitodl/devops ? |
|
This will be part of the AMI build that I'm currently working on and should have in QA today. |
part of #8