-
Notifications
You must be signed in to change notification settings - Fork 88
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
#250: Jib maven plugin added to make the containerization easier. #317
Conversation
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.
github review has some UX problem: I did this review already but was interrupted and never submitted the final result. As it seems until then all my review comments remain invisble for others. Sorry, we are short on time and I wasted some more...
<profile> | ||
<id>docker</id> | ||
<build> | ||
<plugins> | ||
<plugin> | ||
<groupId>com.google.cloud.tools</groupId> | ||
<artifactId>jib-maven-plugin</artifactId> | ||
<version>2.6.0</version> | ||
<configuration> | ||
<from> | ||
<image>adoptopenjdk/openjdk11:jre-11.0.4_11-alpine</image> | ||
</from> | ||
<to> | ||
<image>${project.name}/java:${project.version}</image> | ||
</to> | ||
<container> | ||
<mainClass>com.devonfw.application.mtsj.SpringBootApp</mainClass> | ||
<ports> | ||
<port>8081</port> | ||
</ports> | ||
<format>OCI</format> | ||
</container> | ||
</configuration> | ||
<executions> | ||
<execution> | ||
<phase>package</phase> | ||
<goals> | ||
<goal>dockerBuild</goal> | ||
</goals> | ||
</execution> | ||
</executions> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
</profile> |
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.
shouldnt we put this into the server
module?
This is the only place where it makes sense to build a container from...
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.
Configured the server pom and tested, there is no image created and no error also.
With the parent pom, there are three images getting created for api, core and server. configured api and server pom to skip image creation.
Image created successfully with core and able to run the container..
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.
actually neither api nor core should build the docker image but only server. The docker image must not only build but it also should work (run the application). This can only work for the bootified artifact what is only available in server
.
I can not support this right now but I guess we have some missunderstanding or lack of maven detailed insights knowledge here. Maybe we move this to the next release, even though it is almost complete...
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 image is tested and the application worked. Forgot to mention it in the previous comment. Anyway let's have a detail study and plan for next release.
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.
@sujith-mn Could you please catch up again on this PR? It also needs to be merged with master
.
If you want, you can also reach out and have a call with me to discuss...
<artifactId>jib-maven-plugin</artifactId> | ||
<configuration> | ||
<!-- we don't want jib to execute on this module --> | ||
<skip>true</skip> |
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.
That is what I wrote in my review comment. It is not so good to add it to then parent pom and then having the need to disable (skip) it in all other modules. Please therefore move the plugin only to the server
module that provides the deliverable of the entire app by design.
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.
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.
Alternatively tried different way to build:
- mvn compile jib:build – build is failing, jib plugin is not found. the plugin is configured in the server-pom.
<licenseMerge>Apache Software License, Version 2.0|The Apache Software License, Version 2.0|Apache | ||
2.0|Apache License, Version 2.0</licenseMerge> |
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 is great that you now format all the code.
Sorry, to still bother you...
But is our XML formatter in devonfw IDE missconfigured? Auto-wrapping in XML can cause damage. Are we sure this change will not break anything as maybe then Apache 2.0
will not match anymore (as a newline is now expected).
<image>${project.name}/java:${project.version}</image> | ||
</to> | ||
<container> | ||
<mainClass>com.devonfw.application.mtsj.SpringBootApp</mainClass> |
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 wrong as this is the Classname of the My-Thai-Star application. However, this is the template to create a new application for a project that will reside in a different package. See spring-boot plugin where the same is already configured. For all these things you should consider using variables to avoid redundancies.
<plugin> | ||
<groupId>com.google.cloud.tools</groupId> | ||
<artifactId>jib-maven-plugin</artifactId> | ||
<version>2.6.0</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.
we should always use variables for such versions. This is the only way for us to make devon java migrate
being able to properly update such things.
<version>2.6.0</version> | ||
<configuration> | ||
<from> | ||
<image>adoptopenjdk/openjdk11:jre-11.0.4_11-alpine</image> |
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 IMHO also like a dependency with a version. We need to keep this up-to-date and have our eyes on it to close CVEs in the future. Please also use a variable in toplevel pom of the archetype.
<container> | ||
<mainClass>com.devonfw.application.mtsj.SpringBootApp</mainClass> | ||
<ports> | ||
<port>8081</port> |
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.
Please also use a variable here. We wont need to change this in devon java migrate
in the future but if you use a variable this allows to tweak it easily on the commandline (mvn package -Dapp.container.port=8088
) giving more flexibility if needed.
<ports> | ||
<port>8081</port> | ||
</ports> | ||
<format>OCI</format> |
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 assume there are not many other reasonable options here so we can leave this "hardcoded".
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.
JIB uses exploded deployment to build docker, packaged deployment is not yet support with WAR creation. Thus configuring in the server module may not work as expected.
The other option is to keep the exploded classes from the server module to tomcat container (eg: tomcat:8.5-jre8-alpine).
As Jorge suggested, the regular deployment to external servlet container is discouraged by devon4j. With this configuration we may loose several features of spring boot.
So planning to keep the current JIB configuration.
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.
@sujith-mn Thanks for the rework. Looking quite good now 👍
Remaining questions:
- how could one provide JVM parameters for the container?
- how could one provide a customized
application.properties
for the container?
@maybeec and @maihacke any concerns or feedback (jib bs. spring-boot approach)?
In my opinion, both questions which you raised Jörg could even be ignored. |
I think @maybeec is right, this a general problem which is solvable by spring-boot configs mechanism (overwrite with env params, use spring-cloud-config, ...). We should explain this in the devonfw docs and give hints what to use. |
This is internal port of the app, that can be mapped while running the container. |
Fine. So we could add some hints on this to the documentation and are done. Further we had this discussion about spring-boot-maven-plugin vs. jib. |
https://spring.io/blog/2018/11/08/spring-boot-in-a-container#jib-maven-and-gradle-plugins So it is possible. So what are our missunderstandings then @sujith-mn ? We could have the container build active by default then... |
@hohwille not sure what you mean. I cannot read from the link what you stated in the comment. It's written there as well.... You can bould without docker being installed |
Spring boot maven plugin requires docker demon. https://docs.spring.io/spring-boot/docs/current/maven-plugin/reference/htmlsingle/#build-image |
@sujith-mn told that the build is not working without docker being installed and hence the activation by default has been removed:
This should be clarified:
|
The JIB docker build can be done in 3 ways. 1. Build the image and run locally
2. Build the image and upload to the registery
3. Build the image as tarball
https://github.com/GoogleContainerTools/jib/tree/master/jib-maven-plugin |
If 2. can build the image without docker, why can't it be build without deploying to a docker registry? |
buildTar can build the tar ball and it can pushed to repository manually.
|
Already above I linked this:
So is this a lie or not? Cant be so hard to clarify if this this true or not. |
Conclusion:
|
Implements issue #250