Docker issues by zerdos · Pull Request #104 · coder/code-server

zerdos · 2019-03-07T20:43:55Z

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

zerdos · 2019-03-07T21:19:15Z

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

nhooyr · 2019-03-07T21:40:36Z

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.

nhooyr · 2019-03-07T21:41:11Z

+.gitignore
 Dockerfile
+lib
+node_modules


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.

I guess we can add it back later.

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.

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

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

nhooyr · 2019-03-07T21:42:57Z

@@ -27,172 +27,175 @@
  integrity sha1-7ihweulOEdK4J7y+UnC86n8+ce4=


Why does this file have so many changes?

nhooyr · 2019-03-07T21:43:35Z

  },
  "devDependencies": {
    "@types/tar-stream": "^1.6.0",
+    "cross-env": "^5.2.0",


@kylecarbs why wasn't this here already?

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 should be fixed now (71abb8d); the scripts directly reference the cross-env binary installed in the root node_modules directory.

nhooyr · 2019-03-07T21:49:46Z

@zerdos instead of opening multiple PRs, in the future please edit the existing PR.

nhooyr · 2019-03-07T21:59:10Z

@@ -1,32 +1,47 @@
-FROM node:8.9.3
+FROM node:8.15.1


8.15.1 will not build as nexe does not support it.

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

zerdos · 2019-03-07T23:54:21Z

Removed the yarn.lock file changes, also the other non version bumps.

...and it seems the docker build is still working :)

Omeryl · 2019-03-08T00:42:52Z

 WORKDIR /src
 COPY . .
-RUN yarn
+RUN yarn --forzen-lockfile


This should be --frozen-lockfile

mko-x

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.

mko-x · 2019-03-09T02:45:24Z

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


why an additional starred .DS_Store?
it is not necessary

Suggested change

*.DS_Store

mko-x · 2019-03-09T02:47:20Z

+.DS_Store
+*.DS_Store
+release
+**/yarn-error.log


Shouldn't any logs be ignored?

Suggested change

**/yarn-error.log

**/*.log

mko-x · 2019-03-09T02:49:39Z

 out
 .DS_Store
 release
+yarn-error.log


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

Suggested change

yarn-error.log

**/*.log

… docker

sr229 · 2019-03-16T17:19:33Z

-    "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",


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

zerdos · 2019-03-16T17:23:02Z

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 · 2019-03-16T17:35:20Z

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 · 2019-03-16T17:45:56Z

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.

Zoltan added 10 commits March 7, 2019 08:06

make docker build working with after a fresh checkout

4f0dbd4

upgrade lock file

f1ddb86

update .dockerignore file

c0c7de8

fix typo

b62306e

add more to the .dockerignore

3396cff

add cross-env to vscode bootstrap-fork generator

ec2518a

docker additions

7e44a39

merge back codercom

0c442f2

remove todo

3128602

add better comment for a workaround

4f5e9ab

zerdos requested review from code-asher, kylecarbs and nhooyr as code owners March 7, 2019 20:43

zerdos closed this Mar 7, 2019

zerdos reopened this Mar 7, 2019

nhooyr reviewed Mar 7, 2019

View reviewed changes

add sequential build to docker

c8f3aa5

nhooyr reviewed Mar 7, 2019

View reviewed changes

Comment thread build/tasks.ts Outdated

nhooyr reviewed Mar 7, 2019

View reviewed changes

Comment thread scripts/install-packages.ts

nhooyr reviewed Mar 7, 2019

View reviewed changes

Zoltan added 5 commits March 7, 2019 22:55

remove git modules workaround

035e434

merge back master

f1dc057

node version downgrade to be compatible with nexe

bbf610d

restore yarn.lock

994d5fd

restore package.json

3f6aa4c

Zoltan added 3 commits March 7, 2019 23:45

restore another yarn.lock

b935418

other yarn.lock

b093abd

restore last yarn.lock

a9b825b

Omeryl reviewed Mar 8, 2019

View reviewed changes

kylecarbs reviewed Mar 8, 2019

View reviewed changes

Comment thread Dockerfile

mko-x suggested changes Mar 9, 2019

View reviewed changes

Zoltan added 3 commits March 11, 2019 12:37

merge with the original branch

a4f9e53

Merge branch 'master' of https://github.com/codercom/code-server into…

5ba803f

… docker

merge master in

9982968

sr229 suggested changes Mar 16, 2019

View reviewed changes

zerdos closed this Mar 16, 2019

		@@ -27,172 +27,175 @@
		integrity sha1-7ihweulOEdK4J7y+UnC86n8+ce4=

Conversation

zerdos commented Mar 7, 2019

Uh oh!

zerdos commented Mar 7, 2019

Uh oh!

nhooyr commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhooyr commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zerdos commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mko-x left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zerdos commented Mar 16, 2019

Uh oh!

sr229 commented Mar 16, 2019

Uh oh!

zerdos commented Mar 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nhooyr commented Mar 7, 2019 •

edited

Loading

code-asher Mar 8, 2019 •

edited

Loading