Skip to content
This repository has been archived by the owner on Aug 16, 2024. It is now read-only.

Update gradle mounted volume path to match dockerfile #14

Merged
merged 1 commit into from
Jun 20, 2019

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Jun 18, 2019

What does this PR do?

Modifies the volume used for the .gradle directory to be /home/gradle/.gradle. This is necessary because the dockerfile used specifies this as a VOLUME, which causes issues when that path is not mounted via PVC.

I have tested this devfile and it appears to work (at least, as well as community docker images generally do -- see eclipse-che/che#13454) but further testing is appreciated.

What issues does this PR fix or reference?

eclipse-che/che#13384, see comment for more explanation.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -20,7 +20,7 @@ components:
args: ['infinity']
env:
- name: GRADLE_USER_HOME
value: /home/user/.gradle
value: /home/gradle/.gradle
Copy link
Member

Choose a reason for hiding this comment

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

just a question: do we need two homes? or

      - name: HOME
        value: /home/user

Should be changed to

      - name: HOME
        value: /home/gradle

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point; it's probably better for consistency. The maybe confusing thing about this is that because of arbitrary UID on openshift, the terminal doesn't run as the gradle user, but this still probably makes more sense than /home/user.

In either case, since HOME is not mounted as a volume, any changes there would be wiped out. I wonder if it makes more sense to set HOME to /projects so that at least cd ~ takes users to a meaningful directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

I set to HOME to /home/user originally because that's how legacy Che workspaces worked and there were still some environment variables injected by wsmaster that assumed that. I don't know if that's still the case.

/home/gradle/.gradle is persisted as a volume right? data should not be wiped out.

I am afraid that setting /projects/ as HOME may have unexpected side effects. We may want to set the k8s container workingDir instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@l0rd That makes sense to me but I'm still not sure the best way to proceed here. While /home/gradle/.gradle is persisted, /home/gradle is not; are we okay with this (e.g. it would prevent using a persistent .bashrc, etc.). This issue is probably another, larger discussion, though.

I've updated $HOME to be /home/gradle. What is currently happening, AFAICT, is that whatever directory we're calling $HOME is being created at container start up, so I don't think this will cause issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are ok not to persist full $HOME. This is a cool idea (a user would find his dotfiles in every workspace) but I don't think there has ever been a requirement for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway I am +1 to merge this PR as it is

The community gradle dockerfile expects a volume at

/home/gradle/.gradle

If this volume is not included explicitly, the devfile can fail to start
on kubernetes clusters, since docker will by default attempt to
provision host storage for VOLUMEs. Many Kubernetes clusters forbid
this for security reasons.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk amisevsk merged commit 5a71913 into eclipse-che:master Jun 20, 2019
Ohrimenko1988 added a commit that referenced this pull request May 20, 2020
Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>
Ohrimenko1988 added a commit that referenced this pull request Jun 1, 2020
Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants