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.

Sign up for GitHub

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

Jump to bottom

Modify che-server Dockerfile to run on OpenShift #6200

Merged
merged 3 commits into from
Sep 11, 2017
Merged

Modify che-server Dockerfile to run on OpenShift #6200

merged 3 commits into from
Sep 11, 2017

Conversation

l0rd
Copy link
Contributor

@l0rd l0rd commented Sep 8, 2017

What does this PR do?

Modify Che alpine-based Dockerfile to make it OpenShift compatible.

What issues does this PR fix or reference?

The 6th task of #6097

Changelog

Image eclipse/che-server:nightly can now be run on OpenShift

Release Notes

Image eclipse/che-server:nightly can now be run on OpenShift

@l0rd
Modify che-server Dockerfile to run on OpenShift
98db39c 
Signed-off-by: Mario Loriedo <mloriedo@redhat.com>
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@l0rd l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label Sep 8, 2017
@codenvy-ci
Copy link

Can one of the admins verify this patch?

benoitf
benoitf previously approved these changes Sep 8, 2017
View reviewed changes
dockerfiles/che/Dockerfile Outdated
@@ -38,3 +38,5 @@ COPY entrypoint.sh /entrypoint.sh
COPY open-jdk-source-file-location /open-jdk-source-file-location
ENTRYPOINT ["/entrypoint.sh"]
ADD eclipse-che.tar.gz /home/user/
RUN mkdir /logs /data
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move these RUN instructions before the ADD eclipse-che.tar.gz instruction as it's mainly the only layer that is changing overt time

Copy link
Contributor

@benoitf benoitf Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

and merge the two RUN instructions into only one (merge it to the only one RUN instruction of the file)

dockerfiles/che/Dockerfile.centos Outdated
RUN chmod -R 0777 /home/user/
RUN mkdir /data && chmod 0777 /data
RUN mkdir /logs /data
RUN chmod -R 0777 /logs /data /home/user
Copy link
Contributor

Choose a reason for hiding this comment

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

the same remark that before

@benoitf benoitf dismissed their stale review September 8, 2017 15:32

not yet approved

@l0rd
Fixup after Florent review
1a0deb0 
Signed-off-by: Mario Loriedo <mloriedo@redhat.com>
@l0rd
Copy link
Contributor Author

l0rd commented Sep 8, 2017 •
edited
Loading

@benoitf good points. I've moved mkdir and chmod of /logs and /data on the same line before ADD. But moving chmod -R 777 /home/user/ before ADD is not possible: the folder doesn't exist yet so we need to execute it after the ADD instruction.

@l0rd
Fixup n.2 for newline
124f535 
Signed-off-by: Mario Loriedo <mloriedo@redhat.com>
garagatyi
garagatyi reviewed Sep 8, 2017
View reviewed changes
dockerfiles/che/Dockerfile
ADD eclipse-che.tar.gz /home/user/
RUN chmod -R 0777 /home/user
Copy link

@garagatyi garagatyi Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can we move it to the previous RUN instruction e.g.:

RUN mkdir -p /logs /data /home/user && \
    chmod -R 0777 /logs /data /home/user

Copy link

Choose a reason for hiding this comment

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

Oh, I see that it won't help because you want to affect unpacked archive

@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 11, 2017
@l0rd l0rd merged commit 715a98f into eclipse-che:master Sep 11, 2017
@l0rd l0rd deleted the run-alpine-on-openshift branch September 11, 2017 09:00
@benoitf benoitf added this to the 5.18.0 milestone Sep 11, 2017
@benoitf benoitf added team/osio and removed status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Sep 11, 2017
@l0rd l0rd mentioned this pull request Sep 11, 2017
Closed
@benoitf
Copy link
Contributor

benoitf commented Sep 14, 2017

hello, is this PR have increased the size of the image ?
https://hub.docker.com/r/eclipse/che-server/tags/

nightly     407 MB      14 hours ago
5.17.0      228 MB      22 days ago

@l0rd
Copy link
Contributor Author

l0rd commented Sep 14, 2017

@benoitf yes. We need to find a better solution to have the right permissions on the tar.gz itself.

$ docker history eclipse/che-server:nightly
IMAGE               CREATED             CREATED BY                                      SIZE                COMMENT
07b53940dbd5        14 hours ago        /bin/sh -c chmod -R 0777 /home/user             185MB
<missing>           14 hours ago        /bin/sh -c #(nop) ADD file:297633b39f9900d...   185MB
<missing>           2 days ago          /bin/sh -c mkdir /logs /data &&     chmod ...   0B
<missing>           2 weeks ago         /bin/sh -c #(nop)  ENTRYPOINT ["/entrypoin...   0B
<missing>           2 weeks ago         /bin/sh -c #(nop) COPY file:08ec4490bdad68...   20.6kB
<missing>           2 weeks ago         /bin/sh -c #(nop) COPY file:e9b11779a19eda...   13.6kB
<missing>           2 weeks ago         /bin/sh -c #(nop)  EXPOSE 8000/tcp 8080/tcp     0B
<missing>           2 weeks ago         /bin/sh -c echo "http://dl-4.alpinelinux.o...   21.4MB
<missing>           2 weeks ago         /bin/sh -c #(nop)  ENV LANG=C.UTF-8 DOCKER...   0B
<missing>           6 months ago        /bin/sh -c set -x  && apk add --no-cache  ...   103MB
<missing>           6 months ago        /bin/sh -c #(nop)  ENV JAVA_ALPINE_VERSION...   0B
<missing>           6 months ago        /bin/sh -c #(nop)  ENV JAVA_VERSION=8u111       0B
<missing>           6 months ago        /bin/sh -c #(nop)  ENV PATH=/usr/local/sbi...   0B
<missing>           6 months ago        /bin/sh -c #(nop)  ENV JAVA_HOME=/usr/lib/...   0B
<missing>           6 months ago        /bin/sh -c {   echo '#!/bin/sh';   echo 's...   87B
<missing>           6 months ago        /bin/sh -c #(nop)  ENV LANG=C.UTF-8             0B
<missing>           6 months ago        /bin/sh -c #(nop) ADD file:3df55c321c1c8d7...   4.81MB

@l0rd l0rd mentioned this pull request Sep 14, 2017
Merged
@l0rd
Copy link
Contributor Author

l0rd commented Sep 14, 2017

@benoitf I've opened #6272 that should fix that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoitf benoitf benoitf approved these changes

@ibuziuk ibuziuk ibuziuk approved these changes

@riuvshin riuvshin riuvshin approved these changes

@garagatyi garagatyi garagatyi approved these changes

@sleshchenko sleshchenko sleshchenko approved these changes

Assignees

@l0rd l0rd

Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
5.18.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@l0rd @codenvy-ci @benoitf @ibuziuk @riuvshin @garagatyi @sleshchenko