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

Closed
wants to merge 5 commits into from
Closed

feat: user-mode docker #192

wants to merge 5 commits into from

Conversation

sr229
Copy link
Contributor

@sr229 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 mentioned this pull request Mar 11, 2019
Signed-off-by: Hikari <enra@sayonika.moe>
@sr229 sr229 marked this pull request as ready for review March 13, 2019 17:12
@sr229 sr229 requested a review from nhooyr as a code owner March 13, 2019 17:12
@sr229 sr229 changed the title [DO NOT MERGE] feat: user-mode docker feat: user-mode docker Mar 13, 2019
@kylecarbs
Copy link
Member

Wonderful change.

ping @nhooyr

Dockerfile Show resolved Hide resolved
Copy link
Member

@coadler coadler left a comment

Choose a reason for hiding this comment

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

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

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

@sr229
Copy link
Contributor Author

sr229 commented Mar 16, 2019

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

@davefinster
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
Copy link
Member

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

@sr229
Copy link
Contributor 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.

@davefinster
Copy link

Ignore my comment - turns out it was due to a dirty workspace key in localstorage.

im abusing the word slap today please help me
@sr229
Copy link
Contributor Author

sr229 commented Mar 27, 2019

Request for another review for merge @coadler @kylecarbs

@lsmoura
Copy link
Contributor

lsmoura commented Mar 28, 2019

@sr229 you need to merge master into your branch again. 😓

@sr229
Copy link
Contributor Author

sr229 commented Mar 29, 2019

Should be ready to merge now.

nhooyr added a commit that referenced this pull request Apr 4, 2019
- Adds dumb-init so closes #403, closes #361, closes #383
- User mode docker so closes #192, closes #65
- Uses latest docker ubuntu instead of 18.10 which is the rolling tag so closes #404

Thanks to @RichardMcSorley and @sr229
nhooyr added a commit that referenced this pull request Apr 4, 2019
- Adds dumb-init so closes #403, closes #361, closes #383
- User mode docker so closes #192, closes #65
- Uses latest docker ubuntu instead of 18.10 which is the rolling tag so closes #404

Thanks to @RichardMcSorley and @sr229
nhooyr added a commit that referenced this pull request Apr 4, 2019
- Adds dumb-init so closes #403, closes #361, closes #383
- User mode docker so closes #192, closes #65
- Uses latest docker ubuntu instead of 18.10 which is the rolling tag so closes #404

Thanks to @RichardMcSorley and @sr229
nhooyr added a commit that referenced this pull request Apr 4, 2019
- Adds dumb-init so closes #403, closes #361, closes #383
- User mode docker so closes #192, closes #65
- Uses latest docker ubuntu instead of 18.10 which is the rolling tag so closes #404

Thanks to @RichardMcSorley and @sr229
nhooyr added a commit that referenced this pull request Apr 4, 2019
- Adds dumb-init so closes #403, closes #361, closes #383
- User mode docker so closes #192, closes #65
- Uses latest docker ubuntu instead of 18.10 which is the rolling tag so closes #404

Thanks to @RichardMcSorley and @sr229
@nhooyr nhooyr closed this in #433 Apr 4, 2019
code-asher pushed a commit that referenced this pull request Jun 19, 2019
- Adds dumb-init so closes #403, closes #361, closes #383
- User mode docker so closes #192, closes #65
- Uses latest docker ubuntu instead of 18.10 which is the rolling tag so closes #404

Thanks to @RichardMcSorley and @sr229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sudo in Dockerfile?
7 participants