Add Dockerfile and some cleanup #57
Conversation
packages/runner/src/runner.ts
Outdated
| cwd, | ||
| env: env as NodeJS.ProcessEnv, | ||
| }, log); | ||
| prom.then((result: CommandResult) => { |
There was a problem hiding this comment.
Should return this instead of prom below
There was a problem hiding this comment.
Interesting. It seems to work both ways. Is it a convenience thing?
There was a problem hiding this comment.
You can add
return result;
to the end of that
There was a problem hiding this comment.
Not sure what you mean. To the end of what?
| @@ -44,6 +46,7 @@ export type TaskFunction = (runner: Runner) => void | Promise<void>; | |||
|
|
|||
| export interface Runner { | |||
| cwd: string; | |||
|
|
|||
There was a problem hiding this comment.
Unneeded space, but not a biggie
There was a problem hiding this comment.
Goland formatted automatically, will fix.
Dockerfile
Outdated
| WORKDIR /root/project | ||
| COPY --from=0 /src/packages/server/cli-linux /usr/local/bin/code-server | ||
| EXPOSE 8443 | ||
| # Unfortunately `.` does not work with code-servr |
Dockerfile
Outdated
| RUN yarn task build:server:binary | ||
|
|
||
| # We deploy with ubuntu so that devs have a familiar environemnt. | ||
| FROM ubuntu |
There was a problem hiding this comment.
Pin the version - since this will always resolve to 18.10 (current latest).
| FROM ubuntu | ||
| RUN apt-get update | ||
| RUN apt-get install -y openssl | ||
| WORKDIR /root/project |
There was a problem hiding this comment.
I do not recommend using root. Consider using a actual user or a pseudo-user like what we do in Theia.
There was a problem hiding this comment.
I personally don't really see much value in that. Especially for a dev setup, you pretty much need sudo 100% of the time at which point it just becomes annoying and repetitive.
cc @kylecarbs
There was a problem hiding this comment.
I'm not certain what purpose there'd be in limiting the user when running inside the docker container. What do you think @sr229 ?
There was a problem hiding this comment.
@kylecarbs @nhooyr we're excercising a good practice that the user should be aware that some administrative actions might need sudo or a manual switch to the root. Not all Cloud IDE users are proficient in Linux and know the security implications.
This is also how Theia and Cloud9 does their images.
There was a problem hiding this comment.
Lets get this PR in for now and discuss this in #65.
|
|
||
| # We deploy with ubuntu so that devs have a familiar environemnt. | ||
| FROM ubuntu | ||
| RUN apt-get update |
There was a problem hiding this comment.
Compress these two RUNs as one instruction.
There was a problem hiding this comment.
Its a small thing but I like em separate to make better use of docker's layer cache.
There was a problem hiding this comment.
Optimizing layers no matter how small should be done.
There was a problem hiding this comment.
@nhooyr it is fine to separate them while you are getting the image done, but once it is there, it is strongly recommended (see “apt-get” section) to combine update + install + remove caches
There was a problem hiding this comment.
That is interesting. Does seem reasonable to me to bust the cache every time the package list is modified.
|
|
||
| # Install VS Code's deps. These are the only two it seems we need. | ||
| RUN apt-get update | ||
| RUN apt-get install -y libxkbfile-dev libsecret-1-dev |
There was a problem hiding this comment.
Compress these two RUNs as one instruction as well.
| # directly which should be faster. | ||
| WORKDIR /src | ||
| COPY . . | ||
| RUN yarn |
There was a problem hiding this comment.
compress this as one instruction as well.
Closes #9 #42