Docker issues #104
Docker issues #104
Conversation
zerdos
commented
Mar 7, 2019
zerdos
commented
- add .dockerignore files
- instead of parallel execution, use sequential one (to avoid async errors)
- use locked yarn files (otherwise, we can't be sure which version will be installed)
- use the same node version what we are using on Travis
|
I uploaded an image to dockerhub, which was built from my branch, by dockerhub: |
Lets upgrade the travis version instead @kylecarbs
Lets just do #100
This is a solid change.
This is a good change too. |
| Dockerfile | ||
| lib | ||
| node_modules |
There was a problem hiding this comment.
Why do we want to not put node_modules into the container?
Right now yarn doesn't use it as a cache but it could in the future. See the comment in the Dockerfile.
There was a problem hiding this comment.
I guess we can add it back later.
There was a problem hiding this comment.
https://medium.com/@rdsubhas/docker-for-development-common-problems-and-solutions-95b25cae41eb
In linux we have linux binaries, on mac we have mac binaries, on windows we have exe files.
NPM is a hot mess, so node_modules should be git + docker ignored.
There was a problem hiding this comment.
Are you sure about this? I thought node_modules only contained source code? Not actual binaries.
There was a problem hiding this comment.
@zerdos is right. Check the folder node_modules/.bin/. There are modules that build executables in there (the node-sass package is one that comes to mind. I don't think code-server uses it, but there are many other examples).
yarn.lock
Outdated
| @@ -27,172 +27,175 @@ | |||
| integrity sha1-7ihweulOEdK4J7y+UnC86n8+ce4= | |||
There was a problem hiding this comment.
Why does this file have so many changes?
| @@ -14,6 +14,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "@types/tar-stream": "^1.6.0", | |||
| "cross-env": "^5.2.0", | |||
There was a problem hiding this comment.
@kylecarbs why wasn't this here already?
There was a problem hiding this comment.
I've seen a commit yesterday, which removed the global instal of this packet from travis.
I think you guys have this package installed globally :)
There was a problem hiding this comment.
This should be fixed now (71abb8d); the scripts directly reference the cross-env binary installed in the root node_modules directory.
|
@zerdos instead of opening multiple PRs, in the future please edit the existing PR. |
Dockerfile
Outdated
| @@ -1,32 +1,47 @@ | |||
| FROM node:8.9.3 | |||
| FROM node:8.15.1 | |||
There was a problem hiding this comment.
8.15.1 will not build as nexe does not support it.
|
Removed the yarn.lock file changes, also the other non version bumps. ...and it seems the docker build is still working :) |
Dockerfile
Outdated
| @@ -11,7 +11,7 @@ RUN npm install -g yarn | |||
| # directly which should be faster. | |||
| WORKDIR /src | |||
| COPY . . | |||
| RUN yarn | |||
| RUN yarn --forzen-lockfile | |||
There was a problem hiding this comment.
This should be --frozen-lockfile
| **/dist | ||
| **/out | ||
| .DS_Store | ||
| *.DS_Store |
There was a problem hiding this comment.
why an additional starred .DS_Store?
it is not necessary
| *.DS_Store |
| .DS_Store | ||
| *.DS_Store | ||
| release | ||
| **/yarn-error.log |
There was a problem hiding this comment.
Shouldn't any logs be ignored?
| **/yarn-error.log | |
| **/*.log |
| @@ -4,3 +4,4 @@ dist | |||
| out | |||
| .DS_Store | |||
| release | |||
| yarn-error.log | |||
There was a problem hiding this comment.
Same as with .dockerignore, all log-files should be ignored
| yarn-error.log | |
| **/*.log |
| "packages:install": "cd ./packages && yarn", | ||
| "postinstall": "npm-run-all --parallel packages:install build:rules", | ||
| "packages:install": "cd ./packages && yarn --frozen-lockfile", | ||
| "postinstall": "npm-run-all packages:install build:rules", |
There was a problem hiding this comment.
I want a rationale why the parallel flag have to be removed.
|
I wasn't able to run a successful build without this. |
|
Unfortunately this is a cherry pick not a merge, I don't think that Dockerfile is a pass even for me. It's way too many layers for a build step whoch won't be used anyways. Practice Layer optimization please. |
|
The end result will be small (2 stages build). Those yarn commands could be on one layer for sure, but the build should be sequential - its not running with the parallel execution task unfortunately after a fresh checkout. The biggest difference in the PR, that I put the node_modules directories to the .dockerignore. Btw, on my laptop (mac environment) I never managed to run a successful build from the master branch. |
