Skip to content
This repository has been archived by the owner on May 27, 2021. It is now read-only.

Add Docker Support, closes #68 #69

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add Docker Support, closes #68 #69

wants to merge 6 commits into from

Conversation

hutchgrant
Copy link
Member

Related Issue

closes #68

Summary of Changes

Added a Dockerfile and updated the package.json to include that file.

Dockerfile will:

  1. Built from node:10
  2. Set environment to production
  3. Expose ws port 8000 and test ports
  4. Install latest npm
  5. copy package.json to /opt and install it
  6. set node_modules to path
  7. Copy source code to /opt/app
  8. Create .eslintignore to ignore node_modules
  9. Set permissions so that the ./public folder is created and removed correctly on build
  10. Run as a non-root user
  11. Build
  12. Run local web server

@thescientist13 thescientist13 self-requested a review January 23, 2019 00:44
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!!

Would it make sense to add some documentation in the README around how / when to use Docker, for developers?

Also, would we need to add Dockerfile to files in package.json?

@thescientist13 thescientist13 added the enhancement New feature or request label Jan 23, 2019
@hutchgrant
Copy link
Member Author

Yes good call we would need to update docs and add it to package.json as well

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @hutchgrant ,

Had a chance to try this out a little more hands on, and here's what I saw when running some of the tasks (npm scripts).

npm run test

$ docker run --init imagenamename npm run test

Seems to start ok, but then stalls out and never runs the test.

ℹ 「wdm」: Compiled successfully.
29 01 2019 23:56:06.517:INFO [karma-server]: Karma v3.1.1 server started at http://0.0.0.0:9876/
29 01 2019 23:56:06.517:INFO [launcher]: Launching browsers ChromiumHeadlessConfigured with concurrency unlimited
29 01 2019 23:56:06.522:INFO [launcher]: Starting browser ChromeHeadless
29 01 2019 23:56:06.523:DEBUG [temp-dir]: Creating temp dir at /tmp/karma-73844424
29 01 2019 23:56:06.524:DEBUG [launcher]: /opt/app/node_modules/puppeteer/.local-chromium/linux-499413/chrome-linux/chrome --user-data-dir=/tmp/karma-73844424 --no-default-browser-check --no-first-run --disable-default-apps --disable-popup-blocking --disable-translate --disable-background-timer-throttling --disable-renderer-backgrounding --disable-device-discovery-notifications --no-sandbox --disable-setuid-sandbox http://localhost:9876/?id=73844424 --headless --disable-gpu --remote-debugging-port=9222

npm run develop

$ docker run --init imagenamename npm run develop

Compiles successfully, but my browser times out trying to load http://172.17.0.2:8000/
screen shot 2019-01-29 at 6 54 09 pm

npm run build and npm run lint run fine though 👍

Also, I think maybe in the README we should emphasize this Dockerfile is for development, not necessarily for production. Or could it do both? I think the image is kind of large (given all the dev tools)... Thoughts?

@hutchgrant
Copy link
Member Author

hutchgrant commented Jan 30, 2019

One reason your browser timed out is that IP is for my personal container, yours probably differs. Are you sure you were connecting to the one for your container? I didn't specify that in the readme so I can understand if that caused confusion.

You could use it for dev/qa/prod it should work for all of them.

@thescientist13
Copy link
Member

One reason your browser timed out is that IP is for my personal container, yours probably differs. Are you sure you were connecting to the one for your container? I didn't specify that in the readme so I can understand if that caused confusion.

I was following this from the README

You can access the app in your browser at http://172.17.0.2:8000 (your container's IP at port 8000)

@hutchgrant
Copy link
Member Author

I should have specified when you start the container it lists 3 IPs to connect to the server and only the one for your container works.

@hutchgrant
Copy link
Member Author

Okay I figured out why your develop version didn't work. For one we aren't exposing port 1981 in the dockerfile and two the webpack.config.develop.js has the webpack-dev-server configured to use 'localhost' and not the container's IP.

So readme will have to be modified to point that out. IF you want to run in development, you'll need to make sure you modify the host in the webpack dev config

As for the test, I can run it without issue

I will also modify the instruction to describe how to get your container's IP.

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @hutchgrant .

I think it would be good to use the issue to clarify in what capacity you want to add Docker for this project

  • as a development tool
  • as a production tool
  • both?

I suggest this because as I review this PR, I think it's trying to do both, which is certainly great feature wise, but I think in this current implementation, they contradict each other.

  • The base image is intended to be a development based image, and as such includes a lot of additional packages and tools that would probably not be put into a production image, plus it's not as optimized as it could be. In a JavaScript world, this implementation would be analogous to shipping both dependencies and devDependencies
  • The implementation here approaches Docker usage from the perspective of standing up a server it seems, which yes, is idiomatic usage of Docker for production, for development it's a little more challenging. For development, I find Docker works really well filling the role of a VM, essentially providing developers a shell to run all their commands from instead of having to use their local machine.

So let me know your thoughts, and we can think of what kind of Docker workflow(s) we want to support here

  • developing with Docker (an interactive terminal)
  • production with Docker (an express server to build and host your app with)

I think both are great features to have, but we would probably want to use another approach to our Dockerfile approach, if the goal is to support both. (something like multi-stage builds might help us here)

Let me know your thoughts!

@hutchgrant
Copy link
Member Author

hutchgrant commented Mar 3, 2019

One alternative is to have separate docker files for different environments each with different base images. I'm not sure if this is best practise but you can simply docker build -f dockerfile.dev for development and docker build -f dockerfile.prod for production ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker support
2 participants