Skip to content

Docker issues #104

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

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -1 +1,12 @@
.git
.gitignore
Dockerfile
lib
node_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can add it back later.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

**/dist
**/out
.DS_Store
*.DS_Store
Copy link

Choose a reason for hiding this comment

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

why an additional starred .DS_Store?
it is not necessary

Suggested change
*.DS_Store

release
**/yarn-error.log
Copy link

Choose a reason for hiding this comment

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

Shouldn't any logs be ignored?

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

**/node_modules
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ dist
out
.DS_Store
release
yarn-error.log
Copy link

Choose a reason for hiding this comment

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

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

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

6 changes: 3 additions & 3 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM node:8.15.0
FROM node:8.9.3

# Install VS Code's deps. These are the only two it seems we need.
RUN apt-get update && apt-get install -y \
Expand All @@ -18,7 +18,7 @@ RUN yarn && yarn task build:server:binary
# We deploy with ubuntu so that devs have a familiar environemnt.
FROM ubuntu:18.10
WORKDIR /root/project
COPY --from=0 /src/packages/server/cli-linux /usr/local/bin/code-server
COPY --from=0 /src/packages/server/cli-linux-x64 /usr/local/bin/code-server
EXPOSE 8443
RUN apt-get update && apt-get install -y \
openssl \
Expand All @@ -29,4 +29,4 @@ RUN apt-get install -y locales && \
# configured in /etc/default/locale so we need to set it manually.
ENV LANG=en_US.UTF-8
# Unfortunately `.` does not work with code-server.
CMD code-server $PWD
CMD code-server $PWD
5 changes: 5 additions & 0 deletions build/tasks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ const ensureClean = register("vscode:clean", async (runner) => {

const status = await runner.execute("git", ["status", "--porcelain"]);
if (status.stdout.trim() !== "") {

// this is an uggly workaround - inside docker git clean throws an error message because of this directory
// related to git modules.
await runner.execute("rm", ["-rf", "node_modules/spdlog"]);

const clean = await runner.execute("git", ["clean", "-f", "-d", "-X"]);
if (clean.exitCode !== 0) {
throw new Error(`Failed to clean git repository: ${clean.stderr}`);
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
"description": "Run VS Code remotely.",
"scripts": {
"build:rules": "cd ./rules && tsc -p .",
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

"start": "cd ./packages/server && yarn start",
"task": "ts-node -r tsconfig-paths/register build/tasks.ts",
"test": "cd ./packages && yarn test"
Expand Down
508 changes: 216 additions & 292 deletions packages/app/browser/yarn.lock

Large diffs are not rendered by default.

10 changes: 5 additions & 5 deletions packages/app/common/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
"name": "@coder/app-common",
"main": "src/app.ts",
"dependencies": {
"material-components-web": "^0.44.0",
"react": "^16.8.1",
"react-dom": "^16.8.1"
"material-components-web": "^0.44.1",
"react": "^16.8.4",
"react-dom": "^16.8.4"
},
"devDependencies": {
"@types/react": "^16.8.2",
"@types/react-dom": "^16.8.0"
"@types/react": "^16.8.7",
"@types/react-dom": "^16.8.2"
}
}
Loading