Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker issues #104

Closed
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@zerdos
Copy link

zerdos commented Mar 7, 2019

  • 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
@aslafy-z

This comment has been minimized.

Copy link

aslafy-z commented on .dockerignore in 4f0dbd4 Mar 7, 2019

These lines might be replaced by

**/node_modules
@aslafy-z

This comment has been minimized.

Copy link

aslafy-z commented on Dockerfile in 4f0dbd4 Mar 7, 2019

Typo here: frozen

@zerdos zerdos requested review from code-asher , kylecarbs and nhooyr as code owners Mar 7, 2019

@zerdos

This comment has been minimized.

Copy link
Author

zerdos commented Mar 7, 2019

I uploaded an image to dockerhub, which was built from my branch, by dockerhub:
https://hub.docker.com/r/zerdos/vscode-server/tags

@zerdos zerdos closed this Mar 7, 2019

@zerdos zerdos reopened this Mar 7, 2019

@nhooyr

This comment has been minimized.

Copy link
Collaborator

nhooyr commented Mar 7, 2019

use the same node version what we are using on Travis

Lets upgrade the travis version instead @kylecarbs

#110

instead of parallel execution, use sequential one (to avoid async errors)

Lets just do #100

use locked yarn files (otherwise, we can't be sure which version will be installed)

This is a solid change.

add .dockerignore files

This is a good change too.

Dockerfile
lib
node_modules

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Collaborator

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.

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Collaborator

I guess we can add it back later.

This comment has been minimized.

@zerdos

zerdos Mar 7, 2019

Author

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.

This comment has been minimized.

@nhooyr

nhooyr Mar 9, 2019

Collaborator

Are you sure about this? I thought node_modules only contained source code? Not actual binaries.

This comment has been minimized.

@lsmoura

lsmoura Mar 10, 2019

@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).

Show resolved Hide resolved build/tasks.ts Outdated
yarn.lock Outdated
@@ -27,172 +27,175 @@
integrity sha1-7ihweulOEdK4J7y+UnC86n8+ce4=

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Collaborator

Why does this file have so many changes?

Show resolved Hide resolved scripts/install-packages.ts Outdated
@@ -14,6 +14,7 @@
},
"devDependencies": {
"@types/tar-stream": "^1.6.0",
"cross-env": "^5.2.0",

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Collaborator

@kylecarbs why wasn't this here already?

This comment has been minimized.

@zerdos

zerdos Mar 7, 2019

Author

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 :)

This comment has been minimized.

@code-asher

code-asher Mar 8, 2019

Collaborator

This should be fixed now (71abb8d); the scripts directly reference the cross-env binary installed in the root node_modules directory.

@nhooyr

This comment has been minimized.

Copy link
Collaborator

nhooyr commented Mar 7, 2019

@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

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Collaborator

8.15.1 will not build as nexe does not support it.

https://github.com/nexe/nexe/releases

@zerdos

This comment has been minimized.

Copy link
Author

zerdos commented Mar 7, 2019

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

This comment has been minimized.

@Omeryl

Omeryl Mar 8, 2019

Contributor

This should be --frozen-lockfile

Show resolved Hide resolved Dockerfile
@mko-x
Copy link

mko-x left a comment

I would do some minor changes as you can see t the different comments.

So far so good - but the serial manual execution of the yarn jobs is no good.

**/dist
**/out
.DS_Store
*.DS_Store

This comment has been minimized.

@mko-x

mko-x Mar 9, 2019

why an additional starred .DS_Store?
it is not necessary

Suggested change
*.DS_Store
.DS_Store
*.DS_Store
release
**/yarn-error.log

This comment has been minimized.

@mko-x

mko-x Mar 9, 2019

Shouldn't any logs be ignored?

Suggested change
**/yarn-error.log
**/*.log
@@ -4,3 +4,4 @@ dist
out
.DS_Store
release
yarn-error.log

This comment has been minimized.

@mko-x

mko-x Mar 9, 2019

Same as with .dockerignore, all log-files should be ignored

Suggested change
yarn-error.log
**/*.log
Show resolved Hide resolved Dockerfile
Show resolved Hide resolved Dockerfile
"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",

This comment has been minimized.

@sr229

sr229 Mar 16, 2019

I want a rationale why the parallel flag have to be removed.

@zerdos

This comment has been minimized.

Copy link
Author

zerdos commented Mar 16, 2019

I wasn't able to run a successful build without this.
Yes, its slower and less efficient, but at least running, after a fresh checkout.

@sr229

This comment has been minimized.

Copy link

sr229 commented Mar 16, 2019

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.

@zerdos

This comment has been minimized.

Copy link
Author

zerdos commented Mar 16, 2019

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.

@zerdos zerdos closed this Mar 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.