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

[GH-459] explicitly create a volume and set permission for project dir #471

Merged
merged 1 commit into from
Apr 14, 2019
Merged

[GH-459] explicitly create a volume and set permission for project dir #471

merged 1 commit into from
Apr 14, 2019

Conversation

sr229
Copy link
Contributor

@sr229 sr229 commented Apr 9, 2019

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

This ensures a emptydir volume if the user forgot to mount. This allows OpenShift compatibility and solves
Docker issues.

Is there an open issue you can link to?

This fixes #459 and #469 partially

@sr229 sr229 requested a review from nhooyr as a code owner April 9, 2019 15:50
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

seems reasonable otherwise

@@ -36,9 +36,15 @@ RUN adduser --gecos '' --disabled-password coder && \

USER coder
# We create first instead of just using WORKDIR as when WORKDIR creates, the user is root.
RUN mkdir -p /home/coder/project
RUN mkdir -p /home/coder/project && \
chmod g+rw /home/coder/project;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

@sr229 sr229 Apr 9, 2019

Choose a reason for hiding this comment

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

I need to set the R/W permissions explicitly because I found out on OpenShift hosts, if this is not set, the container would outright crash.

A good supporting evidence of this is OpenShift scrambling UIDs and GIDs, which screws with permissions a little. This is designed to allow you to write there even if you don't own it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still not clear why this is necessary. You shouldn't be able to write if you don't own it, that is correct behaviour. I'm going to revert this @kylecarbs

Copy link
Contributor

@MichaelDesantis MichaelDesantis Apr 17, 2019

Choose a reason for hiding this comment

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

container would outright crash <-- still a little concerned about the possibility of reintroducing this behavior with a revert.

Copy link
Contributor

Choose a reason for hiding this comment

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

container would outright crash <-- still a little concerned about the possibility of reintroducing this possibility with a revert.

Not a concern, don't worry. Its definitely not outright crashing, there is a permission related issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@multishifties I explained it already becuase of how OpenShift handles UIDs and GIDs, which would make /home/coder/project inaccessible practically on the official image. So I guess this would make sense if they revert it, but the official image will never work on OpenShift due to missing permissions

Copy link
Contributor

Choose a reason for hiding this comment

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

becuase of how OpenShift handles UIDs and GIDs,

What does that mean? How does it handle it?

You're not being clear with your concerns, I can't give you a good response.

@sr229
Copy link
Contributor Author

sr229 commented Apr 10, 2019

CC @nhooyr, what do you think

@kylecarbs
Copy link
Member

LGTM

@kylecarbs kylecarbs merged commit dfabc07 into coder:master Apr 14, 2019
@sr229 sr229 deleted the docker-volumes branch April 14, 2019 06:24
code-asher pushed a commit that referenced this pull request Jun 19, 2019
code-asher pushed a commit that referenced this pull request Jun 19, 2019
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.

Docker issue
4 participants