Review this whole doggone thing #7
Conversation
| node_js: | ||
| - "6" | ||
| services: | ||
| - docker |
| const dockerConf = nunjucks.render(NGINX_TEMPLATE_FILE, dockerContext); | ||
| const prodConf = nunjucks.render(NGINX_TEMPLATE_FILE, defaultContext); | ||
|
|
||
| return { dockerConf, prodConf }; |
| - before-you-ship | ||
| - guides | ||
| - from: first-thing | ||
| to: other-thing |
There was a problem hiding this comment.
Er this is the real, "production" pages.yml right? Should the first-thing/other-thing entry actually be in here?
There was a problem hiding this comment.
Actually this isn't "real" yet, ref #6.
| docker-compose build | ||
|
|
||
| clean: | ||
| rm out/*.conf |
There was a problem hiding this comment.
this seems like a pretty simple makefile--have you considered just making them npm script entries? The main downside of make (IMO) is that it actually requires either the XCode command line tools or brew to use--it doesn't come with OS X by default like, say, python does. So you could get rid of the extra dependency by just integrating these into package.json and using npm as your task runner.
It'd also make it easier to run this on Windows but gosh I dunno why anyone would want to do that... 😁
There was a problem hiding this comment.
Yeah, that's a good suggestion. It will reduce tooling and I'm not even using the dependency capabilities of make.
| } | ||
| return c; | ||
| }); | ||
| return configs; |
There was a problem hiding this comment.
I noticed these are plopped right into the regexps in the generated nginx.conf file without any escaping, etc. Do you think this should be documented, or do you think the from and to keys should be validated to e.g. ensure that they conform to a fairly restricted set of characters?
There was a problem hiding this comment.
I'm not very concerned about this since the integration test will fail if anything weird goes in there. Or am I missing a potential problem that wouldn't be caught there?
|
|
||
| const lib = require('../../lib'); | ||
|
|
||
| const HOST = process.env.TARGET_HOST || 'http://localhost:8080'; |
There was a problem hiding this comment.
It seems like this TARGET_HOST env var would also be useful for folks running dinghy or docker-machine or other uncommon docker setups; maybe it's worth documenting in the readme?
| "eslint": "3.16.1", | ||
| "eslint-config-airbnb-base": "^11.1.1", | ||
| "eslint-plugin-import": "2.2.0", | ||
| "faucet": "^0.0.1", |
There was a problem hiding this comment.
I don't know why anyone would ever use Windows, but faucet doesn't seem to work on it. It's ok though, those folks can just run the test files directly and parse the raw TAP output.
There was a problem hiding this comment.
blurf...looks like a known issue tape-testing/faucet#14
I'm just using it because the default tape output is atrocious. I'll look into another formatter.
|
@jseppi this looks great! I made a few comments above but they are pretty low-priority. |
|
Thanks, @toolness! |
|
I've addressed all the comments now, so I'm going to close this out. Thanks again! |
Aw yeah it worked!
First I made
master-review-targetwith:Then I made
master-reviewwith:Then I made this PR.