Review of Code - Githubissues

As a technical reviewer of the book related to this repo, I took some time to run through all of the example code to ensure that it is functional. Here is my review of the code. I've set the items as check boxes so that they can be checked off when addressed. When possible, the links to files include the relevant line number. I also hard-coded the commit hash in master that all these comments are currently referring to.

Thank you for creating this! What an awesome resource this will be to me in the future when I write serverless code. As a whole I am impressed with all the thought put into this codebase. Please note that most of these items are my opinions and that I understand that they may not be the direction you want to with this repo.

Current Status

This is the current status of the major items.

[ ] env file example
[ ] Individual Chapter README.md files.
[ ] Libs Issue
[ ] Database Port Collision
[ ] Database Instances Required
[ ] Postgres Alpine Image
[ ] make clean
[ ] deploy dependent on libs
[ ] Chapter Specific Notes
- [ ] ch2
- [ ] ch3
- [ ] ch5
- [ ] ch6
- [ ] ch7
- [ ] ch8
- [ ] ch9

General Notes

env file example

A basic env example file per chapter that a user can copy, then fill in to get going.

Individual Chapter README.md files.

Right now each chapter either is missing a README.md or has a basic README.md that was generated with the serverless cookie cutter. It would be good to have succinct instructions for how to get started with each chapter example code.

Libs Issue

make libs fails on most chapters when ran a second time due to the line find $(LIBS_DIR) -name '*.pyc' | xargs rm in most Makefiles. The issue is due to a reinstall not creating any *.pyc files. Adding a -f flag to rm would fix this (e.g. find $(LIBS_DIR) -name '*.pyc' | xargs rm -f).

Database Port Collision

Both ch2 and ch3 postgres containers use the same port which makes it difficult to run both at the same time.

Database Instances Required

ch2 and ch3 are required for make shell even if you don't want to run tests (i.e. shell to deploy). Fixing the port collision will make this less of an issue.

Postgres Alpine Image

The alpine variants of the postgres docker images are considerably smaller. It would be nice to not use as much disk space for tests.

`make clean`

It would be nice to have a make clean command that cleans up libs, database containers, etc.jjjj

deploy dependent on libs

I forgot to run make libs before deploying a few times. It would be nice to either require libs to run for each deploy in the Makefile or have some form of a check to ensure libs have been installed before a deploy.

Chapter Specific Notes

ch2

You addressed most of my notes in #12. There are a few notes on that PR.

ch3

[ ] Commented out code in contants.py

ch5

[ ] Missing tests and make test target.
The code for this chapter is hard to deploy for the following reasons:
- [ ] The aws region is hard coded to us-west-2
- [ ] The resize image function name for the invoke call is hard-coded.
- [ ] The bucket name is hard-coded with brianz in it.

ch6

[ ] Missing tests and make test target.
[ ] Link in README.md to create your own twitter app in order to get the twitter env vars needed.
[ ] Default values for dynamodb table read/write iops or link to iops docs in the README.
[ ] Difficult to call tweep.py due to the libs not being available, which results in an error about tweepy being unavailable. You could explain how to setup your PYTHON_PATH in the README. There may be an easier way to fix this as well.

ch7

[ ] Missing tests and make test target.
[ ] "brianz" and "bz" is hard-coded in a lot of resource names making it hard to quickly deploy to your own account and test out.
[ ] The requirements.txt is missing the websocket-client requirement.
[ ] Difficult to call producer.py due to the libs folder not being available, which results in an error about tweepy being unavailable. I had to set the PYTHON_PATH environment variable in the docker image before I could run the producer.
[ ] For some reason the boto3 client for kinesis in the producer required me to set the region_name rather than taking it from the environment (e.g. kinesis = boto3.client('kinesis', region_name=os.environ['AWS_REGION'])). Maybe its due to me running a different version of boto3. Setting the exact version in the requirements.txt could fix this.
[ ] Instructions in the README.md that you need to run the producer.py in the container for anything to happen in AWS.

ch8

[ ] Missing tests and make test target.
[ ] The directory structure is inconsistent with the other chapters where the serverless directory is at the root of the chapter directory.
[ ] Instructions for how to install the serverless-prune-plugin as well as the addition of a package.json for ease of installation.
[ ] Commented out code in the serverless.yml](https://github.com/brianz/serverless-design-patterns/blob/9591014a620d8e2a409f7c9b77595a493ece8edc/ch8/map-reduce/serverless/serverless.yml#L29-L34)
[ ] What is the purpose of lorem-counter.py?
[ ] lorem-counter.py has a function declared twice.
[ ] Bucket names are hard-coded and not based off of the env names.
[ ] Make s3://brianz-mapreduce-enron-emails readable to the world or explain in the README.md how to create the same structure in your own S3 bucket with the enron data.
[ ] There are no requirements in the requirements.txt file which causes normal workflow of make libs, then make deploy break.
[ ] It would be nice to have headers in the csv files.
[ ] Instructions on how to invoke the driver (e.g. sls invoke -s $ENV -f Driver).
[ ] empty-bucket.sh is hard coded to a specific bucket that isn't useful for the user of this repo.

ch9

[ ] Missing tests and make test target.
[ ] It wasn't clear that the required DB_PASSWORD env var is just some arbitrary pretend DB_PASSWORD that should be encrypted by kms.
[ ] It would be nice to have a link to docs on how to get a KMS key setup.
[ ] kms.py would be easier to use if it used the same KEY_ARN env var (it currently uses KMS_KEY_ARN rather than KEY_ARN used elsewhere.
[ ] kms.py would be easier to use if it didn't have the example DB_PASSWORD constant and instead just decrypted the encrypted string from the encrypt function.
[ ] Typo in kms.py. (os.envriron should be os.environ)
[ ] It would be really good to have a function that could be called to generate a Rollbar item. In other words, uncomment rollbar requirement, add a RollbarDivide function decorated with the rollbar decorator.
[ ] Instructions for how to install the serverless-prune-plugin as well as the addition of a package.json for ease of installation.
[ ] Typo in handler.py function log statement: "Initializging" -> "Initializing"
[ ] For some reason I had to set the region in the kms.py. Different version of boto3? Maybe that needs to be locked to a specific version.

brianz / serverless-design-patterns

Review of Code #13