Adds flask lambda API #1
Conversation
There was a problem hiding this comment.
Code looks great... Clean and easy to understand what's going on.
It was also very easy to get a local dev server up and running (other than the minor points I've noted inline).
My only suggestions are:
-
Docstrings on the public modules, functions, classes, and methods in
errors.py,resources.pyandschemas.py. -
Using plurals for the endpoint names as oppose to singular nouns. I always find it's a nicer experience consuming APIs that follow this convention: http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api#restful. When I was testing the API, I instinctively tried
/artists/1/instead of/artist/1/which of course returned a 404. I understand this is just a boilerplate but would be good to establish a standard now so projects that base off this are consistent.
Other than that it's all fantastic, high quality code from what I can see!
README.md
Outdated
| ``` | ||
| Or through docker: | ||
| ``` | ||
| doc run --service-ports app |
README.md
Outdated
|
|
||
| 2. Run development DB | ||
| ``` | ||
| docker-compose run db |
There was a problem hiding this comment.
This doesn't forward port 5432 so running python manage.py db upgrade on next step fails. Running docker-compose up instead works.
config/development.py.sample
Outdated
| @@ -0,0 +1,3 @@ | |||
| DEBUG = True | |||
| SQLALCHEMY_ECHO = True | |||
| SQLALCHEMY_DATABASE_URI = 'postgres://app@localhost/app' | |||
There was a problem hiding this comment.
This doesn't include the password set in docker-compose.yml. Maybe it should be SQLALCHEMY_DATABASE_URI = 'postgres://app:app@localhost/app'? Or alternatively the README updated to to mention the development.py settings file needs to be updated with the PostgreSQL password?
| return paginator | ||
|
|
||
|
|
||
| @api.route('/healthz', methods=('HEAD', 'GET')) |
There was a problem hiding this comment.
yes, it's a kubernetes convention which seems to be the defacto name for that handler now
https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/
Dockerfile
Outdated
| apt-get -y install zip | ||
|
|
||
| COPY requirements.txt /requirements.txt | ||
| RUN pip install -r /requirements.txt |
There was a problem hiding this comment.
I notice Docker will run as root? Is the Dockerfile just for dev or will it be ran on production? If it's just for dev please ignore this comment.
There was a problem hiding this comment.
yes it's just for local development purposes, if we wanted to use it for deployment I would significantly change the container config
|
I like the reference you flagged up for naming. I will work it into the references for our external API spec and we can treat it as a standard from there. |
bin/destroy
Outdated
| fi | ||
| terraform env select "$ENVIRONMENT" | ||
|
|
||
| terraform destroy \ |
| @@ -0,0 +1,53 @@ | |||
| version: '3' | |||
There was a problem hiding this comment.
docker-compose on ecs cluster? shouldnt it be ecs container definition ?
There was a problem hiding this comment.
This is just for local development purposes. The plan is to deploy via AWS lambda.
|
Changes made. Please re-review and sign off if correct. |
Python Lambda API gateway solution using Flask and Terraform.