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

Add Dockerfile and some cleanup #57

Merged
merged 1 commit into from Mar 7, 2019

Conversation

Projects
None yet
6 participants
@nhooyr
Copy link
Collaborator

nhooyr commented Mar 6, 2019

Closes #9 #42

@nhooyr nhooyr requested review from kylecarbs and code-asher Mar 6, 2019

@nhooyr nhooyr force-pushed the nhooyr:docker branch from 4010b20 to 3ff2967 Mar 6, 2019

Show resolved Hide resolved scripts/build.sh
cwd,
env: env as NodeJS.ProcessEnv,
}, log);
prom.then((result: CommandResult) => {

This comment has been minimized.

@kylecarbs

kylecarbs Mar 6, 2019

Member

Should return this instead of prom below

This comment has been minimized.

@nhooyr

nhooyr Mar 6, 2019

Author Collaborator

Interesting. It seems to work both ways. Is it a convenience thing?

This comment has been minimized.

@nhooyr

nhooyr Mar 6, 2019

Author Collaborator

screen shot 2019-03-06 at 6 12 36 pm

Can't.

This comment has been minimized.

@kylecarbs

kylecarbs Mar 7, 2019

Member

You can add

return result;

to the end of that

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Author Collaborator

Not sure what you mean. To the end of what?

This comment has been minimized.

@kylecarbs

kylecarbs Mar 7, 2019

Member

Inside the prom.then block

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Author Collaborator

ah very fancy.

@@ -44,6 +46,7 @@ export type TaskFunction = (runner: Runner) => void | Promise<void>;

export interface Runner {
cwd: string;

This comment has been minimized.

@kylecarbs

kylecarbs Mar 6, 2019

Member

Unneeded space, but not a biggie

This comment has been minimized.

@nhooyr

nhooyr Mar 6, 2019

Author Collaborator

Goland formatted automatically, will fix.

@nhooyr nhooyr referenced this pull request Mar 6, 2019

Open

automate releases #58

@nhooyr nhooyr force-pushed the nhooyr:docker branch from 3ff2967 to ec5d6be Mar 6, 2019

@nhooyr nhooyr requested a review from kylecarbs Mar 6, 2019

@nhooyr nhooyr force-pushed the nhooyr:docker branch 6 times, most recently from c68c30d to 85a59ba Mar 6, 2019

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

This comment has been minimized.

@nol166

nol166 Mar 6, 2019

Collaborator

small typo

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Author Collaborator

Fixed.

@nhooyr nhooyr force-pushed the nhooyr:docker branch 2 times, most recently from a10f52d to 94830b5 Mar 7, 2019

Dockerfile Outdated
RUN yarn task build:server:binary

# We deploy with ubuntu so that devs have a familiar environemnt.
FROM ubuntu

This comment has been minimized.

@sr229

sr229 Mar 7, 2019

Contributor

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

This comment has been minimized.

@sr229

sr229 Mar 7, 2019

Contributor

I do not recommend using root. Consider using a actual user or a pseudo-user like what we do in Theia.

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Author Collaborator

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

This comment has been minimized.

@kylecarbs

kylecarbs Mar 7, 2019

Member

I'm not certain what purpose there'd be in limiting the user when running inside the docker container. What do you think @sr229 ?

This comment has been minimized.

@sr229

sr229 Mar 7, 2019

Contributor

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

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Author Collaborator

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

This comment has been minimized.

@sr229

sr229 Mar 7, 2019

Contributor

Compress these two RUNs as one instruction.

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Author Collaborator

Its a small thing but I like em separate to make better use of docker's layer cache.

This comment has been minimized.

@sr229

sr229 Mar 7, 2019

Contributor

Optimizing layers no matter how small should be done.

This comment has been minimized.

@frol

frol Mar 7, 2019

@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

This comment has been minimized.

@nhooyr

nhooyr Mar 7, 2019

Author Collaborator

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

This comment has been minimized.

@sr229

sr229 Mar 7, 2019

Contributor

Compress these two RUNs as one instruction as well.

# directly which should be faster.
WORKDIR /src
COPY . .
RUN yarn

This comment has been minimized.

@sr229

sr229 Mar 7, 2019

Contributor

compress this as one instruction as well.

@nhooyr nhooyr force-pushed the nhooyr:docker branch 4 times, most recently from 9236339 to aa00298 Mar 7, 2019

@nhooyr nhooyr referenced this pull request Mar 7, 2019

Open

sudo in Dockerfile? #65

@nhooyr nhooyr force-pushed the nhooyr:docker branch from aa00298 to bcc8ad0 Mar 7, 2019

@nhooyr nhooyr marked this pull request as ready for review Mar 7, 2019

@kylecarbs kylecarbs merged commit 17267bd into codercom:master Mar 7, 2019

@nhooyr nhooyr deleted the nhooyr:docker branch Mar 7, 2019

@kylecarbs kylecarbs referenced this pull request Mar 7, 2019

Closed

add dockerfile #42

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.