Merge pull request #214 from seddonym/review-chapter-10

Review chapter 10

Author: hjwp

chapter_10_production_readiness.asciidoc

@@ -47,7 +47,7 @@ Instead, we'll use the popular Gunicorn Python/WSGI server.
 In addition, several options in _settings.py_ are currently unacceptable.
 `DEBUG=True`, is strongly recommended against for production,
 we'll want to set a unique `SECRET_KEY`,
-and as we'll see, other things will come up.
+and, as we'll see, other things will come up.
 
 Let's go through and see if we can fix things one by one.
 
@@ -104,6 +104,12 @@ $ *docker build -t superlists . && docker run \
   -it superlists*
 ----
 
+// DAVID: Incidentally I got the following error:
+// Bind for 0.0.0.0:8888 failed: port is already allocated.
+// Turned out the previous container was still running,
+// I just used the docker kill process you taught me about earlier.
+// Not sure if it's worth including that here, possibly clutter?
+
 
 ==== The FTs catch a problem with static files
 
@@ -192,6 +198,12 @@ $ *docker build -t superlists . && docker run \
 And if you take another manual look at your site, things should look much healthier.
 Let's rerun our FTs to confirm:
 
+// DAVID: Incidentally as per your suggestion Harry I have just been skipping
+// the requirements.txt and instead just amending the pip install in the Dockerfile.
+// If we do that, however, we'll need to make sure readers rebuild the image each time
+// we add a requirement - such as at this point.
+
+
 // JAN: That doesn't work until you install whitenoise to venv on local machine
 
 [role="small-code"]
@@ -208,6 +220,12 @@ Ran 3 tests in 10.718s
 OK
 ----
 
+// DAVID: I got
+// UserWarning: No directory at: /Users/david.seddon/Documents/reviewing/goat-book/src/static/
+//  mw_instance = middleware(adapted_handler)
+// I think we need to move the static folder into src too, in the previous chapter.
+
+
 Phew.  Let's commit that:
 
 [subs="specialcharacters,quotes"]
@@ -463,7 +481,7 @@ TIP: Better to fail hard than allow a typo in an environment variable name to
 
 ==== Setting environment variables inside the Dockerfile
 
-Now let's set that environment variable in our Dockerfile using then `ENV` directive:
+Now let's set that environment variable in our Dockerfile using the `ENV` directive:
 
 [role="sourcecode"]
 .Dockerfile (ch10l007)
@@ -560,7 +578,7 @@ image::images/search-results-400-bad-request.png["Duckduckgo search results with
 `ALLOWED_HOSTS` is a security setting
 designed to reject requests that are likely to be forged, broken or malicious
 because they don't appear to be asking for your site
-(HTTP request contain the address they were intended for in a header called "Host").
+(HTTP requests contain the address they were intended for in a header called "Host").
 
 By default, when DEBUG=True, `ALLOWED_HOSTS` effectively allows _localhost_,
 our own machine, so that's why it was working OK until now.
@@ -641,6 +659,13 @@ CMD gunicorn --bind :8888 superlists.wsgi:application
 ----
 ====
 
+// DAVID: It would be nice to explain the difference between RUN and CMD.
+
+// DAVID: Interestingly when I did this I put the RUN directive after the ENV
+// directive, which led to a KeyError: 'DJANGO_SECRET_KEY' which foxed me for a bit.
+// Might be worth calling out that we're running collectstatic in debug mode.
+
+
 // TODO: gitignore src/static
 
 Well, it was fiddly, but that should get us to passing tests!
@@ -657,6 +682,9 @@ OK
 
 We're nearly ready to ship to production!
 
+// DAVID: It might be worth pointing out what Whitenoise is actually doing.
+// From what I understand, we're still using Django to serve static files.
+
 Let's quickly adjust our gitignore, since the static folder is in a new place:
 
 //0010
@@ -673,7 +701,7 @@ $ *git commit -am "Add collectstatic to dockerfile, and new location to gitignor
 
 === Switching to a nonroot user
 
-TODO: this is definitely a good idea for security, needs writing up.
+TODO: WIP, this is definitely a good idea for security, needs writing up.
 
 Dockerfile should gain some lines a bit like this:
 
@@ -707,7 +735,6 @@ $ *rm src/db.sqlite3*
 $ *touch src/db.sqlite3*  # otherwise the --mount type=bind will complain
 ----
 
-
 Now if you run the tests, you'll see they fail;
 
 [role="small-code"]
@@ -721,6 +748,10 @@ selenium.common.exceptions.NoSuchElementException: Message: Unable to locate
 element: [id="id_list_table"]; [...]
 ----
 
+// DAVID: Got me thinking, I'm not always clear when I need to rebuild the image.
+// I would have thought I might need to do it here, but I didn't. Might be worth
+// explaining in the previous chapter when we do.
+
 And you might spot in the browser that we just see a minimal error page,
 with no debug info (try it manually if you like):
 
@@ -756,7 +787,7 @@ I mean, yes, separating the concepts of handlers and loggers and filters,
 and making it all configurable in a nested hierarchy is all well and good
 and covers every possible use case,
 but sometimes you just wanna say "just print stuff to stdout pls",
-and you wish that configuring the simplest thing was a little easier].
+and you wish that configuring the simplest thing was a little easier.].
 
 Here's pretty much the simplest possible logging config
 which just prints everything to the console (ie standard out).
@@ -784,7 +815,6 @@ Rebuild and restart our container,
 try the FT again (or submitting a new list item manually)
 and we now should see a clear error message:
 
-
 ----
 Internal Server Error: /lists/new
 Traceback (most recent call last):