Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Add Dockerfile and some cleanup #57

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

Merged
kylecarbs merged 1 commit into coder:master from nhooyr:docker
Mar 7, 2019
Merged

Add Dockerfile and some cleanup #57

kylecarbs merged 1 commit into coder:master from nhooyr:docker
Mar 7, 2019

Conversation

Copy link
Contributor

@nhooyr nhooyr commented Mar 6, 2019 •
edited
Loading

Closes #9 #42

yush1ga, kylecarbs, LrWm3, hortinstein, and rajat709 reacted with hooray emoji
cwd,
env: env as NodeJS.ProcessEnv,
}, log);
prom.then((result: CommandResult) => {
Copy link
Member

@kylecarbs kylecarbs Mar 6, 2019

Choose a reason for hiding this comment

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

Should return this instead of prom below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@kylecarbs kylecarbs Mar 7, 2019

Choose a reason for hiding this comment

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

You can add

return result;

to the end of that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@kylecarbs kylecarbs Mar 7, 2019

Choose a reason for hiding this comment

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

Inside the prom.then block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah very fancy.

kylecarbs reacted with rocket emoji

export interface Runner {
cwd: string;

Copy link
Member

@kylecarbs kylecarbs Mar 6, 2019

Choose a reason for hiding this comment

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

Unneeded space, but not a biggie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Goland formatted automatically, will fix.

kylecarbs reacted with thumbs up emoji
@nhooyr nhooyr mentioned this pull request Mar 6, 2019
@nhooyr nhooyr requested a review from kylecarbs March 6, 2019 23:14
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
Copy link
Contributor

Choose a reason for hiding this comment

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

small typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Dockerfile Outdated
RUN yarn task build:server:binary

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

Choose a reason for hiding this comment

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

Pin the version - since this will always resolve to 18.10 (current latest).

nhooyr, kylecarbs, and frol reacted with thumbs up emoji
FROM ubuntu
RUN apt-get update
RUN apt-get install -y openssl
WORKDIR /root/project
Copy link
Contributor

Choose a reason for hiding this comment

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

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

frol and lanrat reacted with thumbs up emoji
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

frol reacted with thumbs down emoji
Copy link
Member

@kylecarbs kylecarbs Mar 7, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

frol reacted with thumbs up emoji
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Compress these two RUNs as one instruction.

frol reacted with thumbs up emoji
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

kylecarbs reacted with thumbs up emoji frol reacted with thumbs down emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimizing layers no matter how small should be done.

sidguptajhs reacted with thumbs up emoji
Copy link

Choose a reason for hiding this comment

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

@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

aslafy-z and nhooyr reacted with thumbs up emoji
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Compress these two RUNs as one instruction as well.

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

Choose a reason for hiding this comment

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

compress this as one instruction as well.

@nhooyr nhooyr mentioned this pull request Mar 7, 2019
@nhooyr nhooyr marked this pull request as ready for review March 7, 2019 02:58
@kylecarbs kylecarbs merged commit 17267bd into coder:master Mar 7, 2019
@nhooyr nhooyr deleted the docker branch March 7, 2019 17:42
@kylecarbs kylecarbs mentioned this pull request Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylecarbs kylecarbs kylecarbs approved these changes

@code-asher code-asher Awaiting requested review from code-asher

+3 more reviewers

@frol frol frol left review comments

@nol166 nol166 nol166 left review comments

@sr229 sr229 sr229 requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /