-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
CHE-2977 "move the core Che Dockerfiles and Dockerfile.centos into /dockerfiles/che #3318
Conversation
@TylerJewell |
Build # 1253 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1253/ to view the results. |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1256/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entrypoint should be updated here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason is that Dockerfile.centos hasn't been updated when the
entrypoint of the default Dockerfile has been updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
fi | ||
|
||
# Copy assembly | ||
cp -r ${ASSEMBLY_DIR} ${DIR}/assembly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue #2977 is to not build anymore the docker images from the root but from each folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building context is always sent (i.e. assembly is always copied)
@@ -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 .` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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
I see two drawbacks:
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 . |
@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. |
@l0rd @TylerJewell
and we've the same lifecycle : building the assembly is building the image as well |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1272/ |
@benoitf @TylerJewell that's a proposal to improve speed of Docker build without sacrificing consistency and neatness 😄 |
I've rewritten all the pull request to merge che.sh and docker.sh into entrypoint.sh |
ok maybe if os is supporting it, I could use hard link of zip assembly and add zip instead of directory in Dockerfile |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we not ready to this: Only way to start Che is through che-server docker image
@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. |
Split this PR on two PRs. First fix some bugs, second remove ability to run native tomcat. |
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. |
|
…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>
@l0rd I've updated to use hard link of files instead of a copy 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 |
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>
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1283/ |
@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>
@skabashnyuk I've updated PR to not remove native scripts |
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>
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?
#2977
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