Adsmutils migration #131
Adsmutils migration #131
Conversation
There was a problem hiding this comment.
This is great! The step in the right direction; I have a few questions for you. The main one:
-
following the changes - using local app context - can you think of a way to standardize how we initialize remote/local microservices? Can we actually treat them both the same way?
-
please check that both local/remote webservices requests contain the same values; adsws checks oauth2 and sets some headers - it then forwards the request to the local/remote microservice. The values/headers should be identical for local/remote requests. Is that the case here?
My further comments are below
adsws/factory.py
Outdated
| instance_relative_config=False, | ||
| static_path=static_path, | ||
| static_folder=static_folder, | ||
| local_config=config |
There was a problem hiding this comment.
you can simplify (remove the if statement) by saying:
local_config = config or {}
adsws/factory.py
Outdated
| app.config.from_object('%s.config' % app.name) | ||
| except (IOError, ImportError): | ||
| app.logger.warning("Could not load object {}.config".format(app.name)) | ||
| __load_config(app, 'from_object', '%s.config' % app.name) |
There was a problem hiding this comment.
readability request: can you document allowed values for the 'method' type (and throw runtime error if somebody gives you 'from_foo' parameter?)
i know that it works and flask probably throws that error; but i like to keep things explicit
requirements.txt
Outdated
| @@ -1,10 +1,11 @@ | |||
| git+https://github.com/adsabs/ADSMicroserviceUtils.git@master | |||
There was a problem hiding this comment.
we should really start to use versions - I've created v1.0.1
btw, adsmutils pulls in newer packages; can you please remove these from the requirements.txt here?
No description provided.