-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
I like this approach better than #23 , at least until its constraints cause us problems when writing code. I think this approach makes the code more self-documenting, as well as generating swagger documentation. I do recognize it's a lot of work to lean into restx. |
I think I like this one more? The alternative feels very tedious, verbose, and error-prone. This one feels like it's more intrusive but I suppose that's a reasonable cost for the benefits :) |
|
||
|
||
def create_app() -> flask.Flask: | ||
app = flask.Flask(__name__) | ||
app.response_class = json_response.JsonResponse |
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.
this is handled magically for us by restx now
|
||
|
||
schema_validator = jsonschema.Draft7Validator(NEW_IMPORT_SCHEMA) |
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.
validation of incoming json is now handled by restx
"in": "header", | ||
"description": "Use your GCP auth token, i.e. `gcloud auth print-access-token`. Required scopes are [openid, email, profile]. Write `Bearer <yourtoken>` in the box." | ||
} | ||
} |
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.
weird hack as described in the description
assert len(dbres) == 1 | ||
assert dbres[0].id == str(resp.get_data(as_text=True)) | ||
assert dbres[0].id == id |
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.
tidied a bunch of tests up here
4c46c56
to
65de828
Compare
@@ -0,0 +1,59 @@ | |||
import flask.testing |
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.
I had to rebase and add swagger for the new health check endpoint, so I wrote a test for it too.
app/health.py
Outdated
|
||
class HealthResponse: | ||
def __init__(self, db_health: bool, rawls_health: bool, sam_health: bool): | ||
self.ok = all([db_health, rawls_health, sam]) |
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.
this should be self.ok = all([db_health, rawls_health, sam_health])
, right?
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.
good catch!
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.
retroactive 👍
This uses flask-restx to integrate Swagger. If you want to see it in action, DM me on Slack and I'll send you the link.
The auth is a bit funky. For our Docker-y services, the Apache proxy handles the oauth dance, so we don't have to think about it. Handling OAuth properly would require endpoints in import-service to do the dance ourselves, which isn't necessary in practice as this will hide behind Orchestration and not need to do the dance. Swagger v3 supports auth-in-header but restx is still only on v2. Thus we have to do a mildly gross thing and tell Swagger that we're authing using an API key, and expect the user to type the word
Bearer
before their token in the Authorize box. Given this Swagger will only be seen by internal developers this seemed like the right time/effort compromise.