CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos into /dockerfiles/che #3318

benoitf · 2016-12-07T17:24:08Z

What does this PR do?

Move Dockerfiles to dockerfiles directory
merge docker.sh and che.sh --> entrypoint.sh

What issues does this PR fix or reference?

Previous behavior

Files were at root folder and we had docker.sh and che.sh

New behavior

Files are in dockerfiles folder
also docker.sh script is moved from the assembly folder to the docker image folder and named entrypoint.sh

Change-Id: I7404f8f2896e8a7eea83ecab57a6c4c623c68abe
Signed-off-by: Florent BENOIT fbenoit@codenvy.com

benoitf · 2016-12-07T17:25:51Z

@TylerJewell
@riuvshin it will impact CI jobs of building the image (it also bring a build.sh script -> requires to have called mvn clean install in assembly/assembly-main)
@dharmit / @l0rd it should have impact on CI job of centos as Dockerfile is moved to /dockerfiles subdirectory

codenvy-ci · 2016-12-07T21:52:03Z

Build # 1253 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1253/ to view the results.

codenvy-ci · 2016-12-08T09:29:52Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1256/

l0rd

Thank you for the heads up on that @benoitf. We will handle the necessary modifications for centOS containers pipeline. You can move the Dockerfiles in the meantime.

l0rd · 2016-12-08T09:38:08Z

dockerfiles/che/Dockerfile.centos

@@ -46,7 +46,7 @@ EXPOSE 8000 8080

 USER user

-ADD assembly/assembly-main/target/eclipse-che-*/eclipse-che-* /home/user/che/
+ADD assembly /home/user/che

 ENTRYPOINT [ "/home/user/che/bin/che.sh", "-c" ]


entrypoint should be updated here too

why should I update the entrypoint ? the current dockerfile is not using docker.sh script (which is now entrypoint.sh) so maybe there were some reasons ?

I think the reason is that Dockerfile.centos hasn't been updated when the
entrypoint of the default Dockerfile has been updated.

ok, I wanted to keep Dockerfile.centos as it was but if you say it's a mistake, then I will add usage of entrypoint.sh

l0rd · 2016-12-08T09:42:33Z

dockerfiles/che/build.sh

+fi
+
+# Copy assembly
+cp -r ${ASSEMBLY_DIR} ${DIR}/assembly


Instead of copying the assembly each time (and Docker will copy it one more time) I would rather:

docker build -t eclipse/che-server -f dockerfiles/che/Dockerfile .

from the repository root

the issue #2977 is to not build anymore the docker images from the root but from each folder

As a later improvement I would like to copy assembly only if folder has been updated

BTW relaunching twice the build.sh command if assembly has not been changed is reusing cache layer

Building context is always sent (i.e. assembly is always copied)

l0rd · 2016-12-08T09:47:43Z

dockerfiles/che/Dockerfile

@@ -9,7 +9,7 @@
 #   Codenvy S.A
 #
 # To build it, run in the repository root:
-#  `docker build -t codenvy/che-server .`
+#  `docker build -t eclipse/che-server .`


This doesn't work anymore. I would rather:

docker build -t eclipse/che-server -f dockerfiles/che/Dockerfile .

from the root of the repo.
Dockerfile could copy assembly from assembly/assembly-main/target/eclipse-che-*/eclipse-che-* as before.

the issue #2977 is to not build anymore the docker images from the root but from each folder
the build.sh script is handling that. Maybe I should drop the build instruction from the dockerfile by using docker build but only using build.sh

l0rd · 2016-12-08T12:28:28Z

@benoitf @TylerJewell don't get me wrong, I think that moving dockerfiles in a specific folder is a great choice that will make the folder organization a lot cleaner. However I don't understand why you want to

not build anymore the docker images from the root but from each folder

I see two drawbacks:

The Dockerfile build would be a two step operation (copy + docker build) instead of one. It takes longer, there are more chances to make a mistake, it's not the standard way to go and it's more complicated.
The copy of the assembly is usually the most expensive operation in the che-server Dockerfile build. We would do the most expensive operation of the build one more time with no particular benefit.

And I don't see any benefit compared to one build command from the root folder:

docker build -t eclipse/che-server -f dockerfiles/che/Dockerfile .

TylerJewell · 2016-12-08T15:24:49Z

@l0rd - this just comes down to having a more consistent framework for building. We have done a lot of effort so that every /dockerfiles sub-folder has a consistent build experience on mac, windows, or linux.

And so it just comes down to making the build experience better, and then a side benefit is that the root of the repository is cleaner.

benoitf · 2016-12-08T16:23:08Z

@l0rd @TylerJewell
what if I merge assembly/assembly-main into dockerfiles/che
building che will be done in a single self-contained folder and running che-server will be always done with docker so it should match both of the expectations:

self-contained folder
no recopy of files/folder as assembly and docker images are produced in the same root folder

and we've the same lifecycle : building the assembly is building the image as well
so we're sure we're always in sync locally and don't forget to build image after building assembly

TylerJewell · 2016-12-08T17:48:36Z

As we discussed in slack a little bit, I think we have to have an /assembly/assembly-main/target output. Even if this is unrunnable, it is still a packaged assembly that is docker-ready.
We can then optionally push that assembly into /dockerfiles/che, or we can have the build script in /dockerfiles/che check & pull a copy from the assembly location.
Finally, separately, we could consider adding a flag for the mvn install phase that will have the assembly/assembly-main also build the Docker image from within the assembly, so that it is built, though I would not know how to do that with the Dockerfile for this image located in another location.

codenvy-ci · 2016-12-09T10:54:14Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1272/

l0rd · 2016-12-09T10:58:32Z

@benoitf @TylerJewell that's a proposal to improve speed of Docker build without sacrificing consistency and neatness 😄

#3336

benoitf · 2016-12-09T15:21:46Z

I've rewritten all the pull request to merge che.sh and docker.sh into entrypoint.sh
it also solve the issue #3324

benoitf · 2016-12-09T16:58:17Z

ok maybe if os is supporting it, I could use hard link of zip assembly and add zip instead of directory in Dockerfile
then there will be no copy in that case
It will require some changes to rename directory or have an alias but it may work as well

l0rd · 2016-12-09T18:38:45Z

Hard link to the .zip file should be faster than the copy of the folder. And would take less space on disc too. I'm not sure that's easy to do on Windows tough.

skabashnyuk

we not ready to this: Only way to start Che is through che-server docker image

TylerJewell · 2016-12-09T20:20:25Z

@skabashnyuk - you cannot reject a PR without providing a list of actions that need to be taken. Please itemize whether any additional testing is needed.

skabashnyuk · 2016-12-09T20:25:54Z

Split this PR on two PRs. First fix some bugs, second remove ability to run native tomcat.

TylerJewell · 2016-12-09T20:27:31Z

I would prefer not to do that. The removing of Tomcat is a clean way to solve the bugs. Please itemize anything in CI or QA systems we should build to be ready to remove Tomcat as a native approach.

skabashnyuk · 2016-12-09T20:30:35Z

Dev team have to say that it is able to do all regular development that it can do with current tomcat
We need to have at least a couple of weeks without major outages with the approach how we run che and codenvy with docker.

TylerJewell · 2016-12-09T20:32:36Z

Agree on #1. We will not wait for #2 - that is not a goal that I have.

…ockerfiles/che" merge docker.sh and che.sh. Only way to start Che is through che-server docker image also docker.sh script is moved from the assembly folder to the docker image folder and named entrypoint.sh Change-Id: I7404f8f2896e8a7eea83ecab57a6c4c623c68abe Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

…into /dockerfiles/che" Change-Id: I28e31ee7e8852acb9c2850640c61977a8890a6df Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf · 2016-12-11T17:51:59Z

@l0rd I've updated to use hard link of files instead of a copy
I've also tested hard link on a Windows 10 VM and with git bash it works on my side.

It also now using the path of the internal archive (then directory /home/user/che is changed to /home/user/eclipse-che-5.0.0-M9-SNAPSHOT) as it is using the .tgz file for the hard link

codenvy-ci · 2016-12-11T18:40:02Z

Build # 1282 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1282/ to view the results.

…into /dockerfiles/che" Change-Id: Ic6f90d2f47b19786f7c7b0ad0ad9d820474f8847 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

codenvy-ci · 2016-12-12T08:35:42Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1283/

l0rd · 2016-12-12T10:26:19Z

@benoitf I approved your PR and will close #3336. I still believe that using the repository root as the build context is a better solution than using an hard link. But I understand that you want to use the same convention for every image (build context is dockerfile folder) and I must admit that I'm the only one defending my position so I won't block on that.

…into /dockerfiles/che" Change-Id: I3cd89994c94ab7fdb70b9c27522af2838c61a2c9 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

…into /dockerfiles/che" Change-Id: I95bc276b6ae926fb7b5310edb19011de93766eb2 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf · 2016-12-12T15:09:33Z

@skabashnyuk I've updated PR to not remove native scripts
it will be in a separate pull request

codenvy-ci · 2016-12-12T17:40:21Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1291/

…ockerfiles/che (eclipse-che#3318) * CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos into /dockerfiles/che" merge docker.sh and che.sh. also docker.sh script is moved from the assembly folder to the docker image folder and named entrypoint.sh Change-Id: I7404f8f2896e8a7eea83ecab57a6c4c623c68abe Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

TylerJewell approved these changes Dec 7, 2016

View reviewed changes

benoitf requested review from l0rd and riuvshin December 8, 2016 06:43

benoitf force-pushed the CHE-2977-c branch from 13dae4f to 8477cc7 Compare December 8, 2016 08:32

riuvshin approved these changes Dec 8, 2016

View reviewed changes

l0rd requested changes Dec 8, 2016

View reviewed changes

benoitf force-pushed the CHE-2977-c branch from 8477cc7 to a0b6b7b Compare December 9, 2016 10:04

benoitf mentioned this pull request Dec 9, 2016

Che container creates folders with projects in /data on host machine #3324

Closed

skabashnyuk requested changes Dec 9, 2016

View reviewed changes

benoitf added 2 commits December 11, 2016 18:49

fixup! CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos …

85906b1

…into /dockerfiles/che" Change-Id: I28e31ee7e8852acb9c2850640c61977a8890a6df Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf force-pushed the CHE-2977-c branch from a0b6b7b to 85906b1 Compare December 11, 2016 17:49

benoitf mentioned this pull request Dec 11, 2016

CHE-2977 Changes to allow build from root of repository #3336

Closed

fixup! CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos …

b89c403

…into /dockerfiles/che" Change-Id: Ic6f90d2f47b19786f7c7b0ad0ad9d820474f8847 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

l0rd approved these changes Dec 12, 2016

View reviewed changes

benoitf added 2 commits December 12, 2016 16:02

fixup! CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos …

8b1f0f9

…into /dockerfiles/che" Change-Id: I3cd89994c94ab7fdb70b9c27522af2838c61a2c9 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

fixup! CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos …

a63fafe

…into /dockerfiles/che" Change-Id: I95bc276b6ae926fb7b5310edb19011de93766eb2 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

skabashnyuk approved these changes Dec 12, 2016

View reviewed changes

benoitf merged commit a7d4bb7 into master Dec 12, 2016

benoitf deleted the CHE-2977-c branch December 12, 2016 16:09

nsymms mentioned this pull request Dec 26, 2016

Workspace storage location not inherited from server container #3514

Closed

This was referenced Jan 17, 2017

Che nightly does not start on Windows 10 #3686

Closed

Remove native launch scripts of Eclipse Che (replaced now by Docker CLI) #3780

Merged

benoitf added this to the 5.0.0 milestone Jan 23, 2017

benoitf mentioned this pull request Jan 31, 2017

Remove native launch scripts of Eclipse Che (replaced now by Docker CLI) #3960

Merged

CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos into /dockerfiles/che #3318

CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos into /dockerfiles/che #3318

Conversation

benoitf commented Dec 7, 2016 • edited

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

benoitf commented Dec 7, 2016

codenvy-ci commented Dec 7, 2016

codenvy-ci commented Dec 8, 2016

l0rd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rd commented Dec 8, 2016 • edited

TylerJewell commented Dec 8, 2016

benoitf commented Dec 8, 2016

TylerJewell commented Dec 8, 2016

codenvy-ci commented Dec 9, 2016

l0rd commented Dec 9, 2016

benoitf commented Dec 9, 2016

benoitf commented Dec 9, 2016

l0rd commented Dec 9, 2016

skabashnyuk left a comment

Choose a reason for hiding this comment

TylerJewell commented Dec 9, 2016

skabashnyuk commented Dec 9, 2016 • edited

TylerJewell commented Dec 9, 2016

skabashnyuk commented Dec 9, 2016

TylerJewell commented Dec 9, 2016

benoitf commented Dec 11, 2016 • edited

codenvy-ci commented Dec 11, 2016

codenvy-ci commented Dec 12, 2016

l0rd commented Dec 12, 2016

benoitf commented Dec 12, 2016

codenvy-ci commented Dec 12, 2016

benoitf commented Dec 7, 2016 •

edited

l0rd commented Dec 8, 2016 •

edited

skabashnyuk commented Dec 9, 2016 •

edited

benoitf commented Dec 11, 2016 •

edited