-
Notifications
You must be signed in to change notification settings - Fork 106
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
[WIP] upgrade flask #5508
[WIP] upgrade flask #5508
Conversation
) | ||
if env.get_credential("PRODUCTION"): | ||
if env.get_credential("PRODUCTION") or env.get_credential("STAGE"): |
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.
Do we want to have api_key field in Stage as well?
Codecov Report
@@ Coverage Diff @@
## develop #5508 +/- ##
===========================================
+ Coverage 85.50% 85.58% +0.08%
===========================================
Files 81 81
Lines 8173 8227 +54
===========================================
+ Hits 6988 7041 +53
- Misses 1185 1186 +1
|
@@ -44,9 +44,9 @@ class Resource(six.with_metaclass(MethodResourceMeta, restful.Resource)): | |||
|
|||
|
|||
API_KEY_ARG = fields.Str( | |||
required=True, missing="DEMO_KEY", description=docs.API_KEY_DESCRIPTION, | |||
missing="DEMO_KEY", description=docs.API_KEY_DESCRIPTION, |
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.
required = True caused an error when pushing to PROD. I was not able to reproduce this locally. But I tested using just missing and it worked on Stage.
I think the error is due to a change in webargs where multiple calls to use_kwargs causes a bug when one of them is required. Here is a comment mentioning this. Our situation is not exactly the same, but I think it's the same error.
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.
Great job! Thanks @tmpayton
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.
Great work! @tmpayton
Summary (required)
This PR upgrades flask. This is a continuation of the previous PR.
The only unreviewed changes are in utils.py
Required reviewers 2-3 Devs
Impacted areas of the application
Related PRs
Related PRs against other branches:
How to test
pip install -r requirements.txt
snyk test --file=requirements.txt --package-manager=pip
pytest
flask run