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

feat: user-mode docker #192

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@sr229
Copy link

sr229 commented Mar 11, 2019

Describe in detail the problem you had and how this PR fixes it

Prior to discussion to #65, this runs all file operations in the user coder in container along with some refactoring of the Dockerfile to make it easier to read.

Is there an open issue you can link to?

This resolves #65. This will be blocked until general consensus that this merit as a merge.

@avelino avelino referenced this pull request Mar 11, 2019

Open

sudo in Dockerfile? #65

feat: user-mode docker
Signed-off-by: Hikari <enra@sayonika.moe>

@sr229 sr229 force-pushed the sr229:docker-user branch from d618b09 to b5e0a66 Mar 12, 2019

@sr229 sr229 marked this pull request as ready for review Mar 13, 2019

@sr229 sr229 requested a review from nhooyr as a code owner Mar 13, 2019

@sr229 sr229 changed the title [DO NOT MERGE] feat: user-mode docker feat: user-mode docker Mar 13, 2019

@kylecarbs

This comment has been minimized.

Copy link
Member

kylecarbs commented Mar 14, 2019

Wonderful change.

ping @nhooyr


COPY --from=0 /src/packages/server/cli-linux-x64 /usr/local/bin/code-server
RUN apt-get update && apt-get install -y \
sudo \

This comment has been minimized.

@nhooyr

nhooyr Mar 14, 2019

Collaborator

Please don't combine this. Minimizing layers in favour of readability is premature optimization.

#183 (comment)

This comment has been minimized.

@nhooyr

nhooyr Mar 14, 2019

Collaborator

By combine I mean the echo and adduser stuff.

Its fine to install all deps in a single RUN.

This comment has been minimized.

@sr229

sr229 Mar 15, 2019

Author

I don't think it merits its own RUN? It's better to have it on one build step already than to force it in another layer when they do the exact same thing.

On a novice Linux user's perspective, which I also asked from a couple of Linux beginners, this is still readable, since you can tell after apt the next step is adding user and changing modes, which most are acquainted at already. Which defeats your "Minimzing layers in favor of readability" point. If you really want unreadable Docker images, look further more into the official library images.

This chain is actually more innocent than the rest of how the Docker library images are done (where compliancy is a must). And to validate my claim this is readable in most beginners' perspective, I asked people to read it and ask if they can read and describe me how everything works (which 94% of the people agreeing this is readable).

This comment has been minimized.

@sabrehagen

sabrehagen Mar 19, 2019

This command has three purposes: installing dependencies, adding a user, and configuring application state. None of those three should be in the same step; they are all distinct, unrelated operations, and should be split into three RUN commands.

@coadler
Copy link
Collaborator

coadler left a comment

I think this is a good change and should debate the semantics on an issue later. This seems to be the last step for code-server integration in Che so it'd be nice for people to be able to start using it.

The adduser portion doesn't look that bad to me though, although I know that's not the whole argument. It's minor enough we can reevaluate later

@kylecarbs

This comment has been minimized.

Copy link
Member

kylecarbs commented Mar 15, 2019

Once the conflicts are resolved I'm good to merge @sr229

@sr229

This comment has been minimized.

Copy link
Author

sr229 commented Mar 16, 2019

merge issues fixed, CI seems stuck but PR build reports fine

@davefinster

This comment has been minimized.

Copy link

davefinster commented Mar 18, 2019

I feel like this is worth mentioning, but I made my own changes to shift over to a non-root user that mimic the changes in this PR. The Dockerfile in question is here: https://gist.github.com/davefinster/39825c0fa7cf168b8114d24bd3b3df53

When I first deployed the container and everything worked except Workspace creation. The container was setup via Kube using these params:

command: ["code-server"]
args:
  - --allow-http
  - --no-auth
  - --port=8440
  - --data-dir=/home/coder/.code-server
  - /home/coder
workingDir: /home/coder

Despite that, I was getting file write errors (due to permissions - non-root user writing into /root so not surprising) whenever VS Code tried to anything Workspace related since it was always attempting to write into /root/.code-server/Workspaces/number/workspace.json regardless of what I did. I ended up having to add these lines to get it to work:

mkdir -p /root/.code-server/Workspaces && \
chown -R 3000 /root/.code-server && \
chmod -R 777 /root/.code-server && \
chmod 755 /root
@kylecarbs

This comment has been minimized.

Copy link
Member

kylecarbs commented Mar 19, 2019

@davefinster interesting. @sr229 have you experienced that with this dockerfile?

@sr229

This comment has been minimized.

Copy link
Author

sr229 commented Mar 19, 2019

@davefinster @kylecarbs This is a mirror of my Dockerfile, particularly the Vanilla dockerfile. Every operation works, and you shouldn't be mounting on /root, rather in /home/coder on my end.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.