-
Notifications
You must be signed in to change notification settings - Fork 219
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
XDOCKER-8: Also offer an alpine-based image to reduce download size #20
base: master
Are you sure you want to change the base?
Conversation
Hi. The commit message should have contained also the issue title, |
-> mysql implementation
11/mysql-alpine-tomcat/Dockerfile
Outdated
# _/ /'`\ \_ \ /\ / | | | |`\ \ | | | ||
# |____||____| \/ \/ [___][__| \_][___] | ||
|
||
MAINTAINER Vincent Massol <[email protected]> |
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 should be changed as I'm not planning to be the maintainer for it ;) @ASHISH932 Since you're contributing this are you ok to also maintain it on the long run? Thx
Thanks @ASHISH932 . There's a problem with this PR: it's not using the template mechanism and this contains a lot of duplicated code, which is really not nice for maintainance. Could you refactor it to use the template and build mechanism with Gradle as it's done for the other images? Thx! |
FTR it would also allow me to review more easily this PR to see what are the differences with the other image files, without the duplication :) |
Thanks, I would be doing the changes, could you help me by reviewing my next 6-7 issues, so that I could understand the complete process and code base properly, then I could maintain this image. |
FTR
If you want I could restart with the template? |
Just checked your PR and it's still not correct since it's still not using the template mechanism :) |
* Modified template file to genrate alpine image * Modified Dockerfile to support alpine os * gradlew generated new files with new naming convention * name of container folder now is db-servelet-image (earlier db-servelet) e.g. mysql-tomcat-alpine
Added alpine to the template. But for that naming convention of folders are changed. FTR |
I think we should remove mysql-tomcat and postgres-tomcat directory as we have a replacement now mysql-tomcat-debian and postgres-tomcat-debian. I have not removed it in this commit. If we remove the files then we also need to update it in official-image repo |
FROM tomcat:8-jdk8-slim | ||
|
||
<% if (image == 'debian') print 'FROM tomcat:8-jdk8-slim' | ||
else if (image == 'alpine') print 'FROM tomcat:8-jre8-alpine' %> |
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'm wondering if using a jre instead of a jdk wouldn't cause trouble.
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 have tested the app was working fine, but if you have other concerns I would try to change it?
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.
Well, we were using a JDK and you changed it to a JRE so there got to be a reason :) It's weird to use a JDK for "debian" and a JRE for "alpine". It raises questions as to why we're doing this....
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.
Basically we were using jar and war files. So I thought there is no need for JDK as we don't want to compile anything. That's why I choose JRE.
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.
JRE should be sufficient to run Java apps. The package and image layer will be smaller 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.
JRE should be fine, yes.
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.
All variants should use a jre to run the software. You might only need the jdk to build the jar/war.
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.
If only a JRE is needed (still not sure it's the case and there might be some 3rd party lib we use that would require a JDK. As an example, back in the days - don't know if it's still the case or not -, if you use JSPs you needed a JDK since it had to compile the JSP to java code), then indeed we should change all images. We also need to update https://dev.xwiki.org/xwiki/bin/view/Community/SupportStrategy/JavaSupportStrategy/ to explain that only a JRE is needed for running XWiki.
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 XWiki Debian package depends on openjdk-11-jre-headless
so yes, the JRE is definitely enough, and it's like this since we have Debian packages.
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.
unzip \ | ||
procps | ||
|
||
RUN curl -L -o /mysql-connector-java-5.1.34.jar https://repo1.maven.org/maven2/mysql/mysql-connector-java/5.1.34/mysql-connector-java-5.1.34.jar |
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 is potentially a problem because it requires quite some maintenance. Isn't there a way to use the "latest" version?
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.
What about postgresql?
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 searched for the way to add "latest " version or to add through apk but I was unable to find the solution. I just found a bit better way that is to maintain the version through arguments.
https://github.com/LasLabs/docker-alpine-jira/blob/master/Dockerfile
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.
It only contains MySQL for now. The changed naming convention of variants in build.gradle supports it.
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 searched for the way to add "latest " version or to add through apk but I was unable to find the solution. I just found a bit better way that is to maintain the version through arguments.
Did you commit anything about this? What's your plan?
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 we should maintain it with version. Should I commit that?
@@ -52,15 +66,17 @@ RUN rm -rf /usr/local/tomcat/webapps/* && \\ | |||
mkdir -p /usr/local/tomcat/temp && \\ | |||
mkdir -p /usr/local/xwiki/data && \\ | |||
curl -fSL "\${XWIKI_URL_PREFIX}/xwiki-platform-distribution-war-\${XWIKI_VERSION}.war" -o xwiki.war && \\ | |||
echo "\$XWIKI_DOWNLOAD_SHA256 xwiki.war" | sha256sum -c - && \\ | |||
echo "\$XWIKI_DOWNLOAD_SHA256 xwiki.war" | sha256sum -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.
why the extra space? typo? :)
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.
In alpine, the extra space is required for the sha256 to work. Without space it was failing
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.
@ASHISH932 I think we're not talking about the same thinf. This is just the echo command. I don't see why it would require 2 spaces, it doesn't make sense to me :)
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 know that felt weird to me also, but that's how it works. See this issue.
gliderlabs/docker-alpine#174
procps \\''' %> | ||
<% if (db == 'mysql' && image == 'debian') print 'libmysql-java && \\' | ||
if (db == 'postgres' && image == 'debian') print 'libpostgresql-jdbc-java && \\' %> | ||
<% if (image == 'debian') print 'rm -rf /var/lib/apt/lists/*' %> |
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.
Is this needed since there's alreayd the IF above about debian?
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.
These IF are not nested. The second IF starts after the first IF end
Alpine based XWIKI image