search

fabric8io-images / s2i

OpenShift S2I images for Java and Karaf applications
Apache License 2.0
70 stars 84 forks source link

assemble broken for Spring Boot and devtools since 2.3.0 #171

Open jeffmaury opened 5 years ago

jeffmaury commented 5 years ago

The s2i assemble script is supposed to explode the Spring Boot fat JAR if Spring Boot devtools dependency is found. It is broken since 2.3.0 as unzip is not found anymore in the image.

vorburger commented 5 years ago

@jeffmaury I'm interested in helping with this (unless you're about to raise a PR with a fix?).

This, like e.g. #169, may have been an impact of @rhuss's #165, and in the move FROM jboss/base-jdk:8 to centos we lost unzip in the image? I can put together a PR to add that back in, but...

... ideally, I would like to use this as the opportunity to learn more about the feature behind this, where the s2i assemble script explodes the Spring Boot fat JAR if Spring Boot devtools dependency is found ... do we have doc about this? Interested, because just the other day @edewit and I were talking how something like https://github.com/GoogleContainerTools/skaffold-tools-for-java (see https://github.com/GoogleContainerTools/skaffold) for OpenShift would like like... but sounds like we may already have this? If I can learn more about it, we can make sure it doesn't break anymore here in the future.

vorburger commented 5 years ago

@jeffmaury are you having the problem with the centos or the rhel variant of the base image?

vorburger commented 5 years ago

Oh, just found https://github.com/fabric8io-images/s2i/tree/master/java/images/centos#spring-boot-automatic-restarts from #81 by @dhirajsb ... let me see if I can add an examples/spring-devtools (alongside my earlier maven, maven-wrapper, binary, gradle) which showcases, and integration tests this (for future non-regression).

vorburger commented 5 years ago

@jeffmaury (or @dhirajsb or anyone else familiar with this stuff .. and FYI @edewit) I've toyed with this a bit and come up with #173 BUT (a) I cannot yet reproduce anything broken due to a missing unzip; (b) I don't really fully understand yet how it's supposed to work anyway...

I could debug it further by looking at what #81 did in assemble, but it feels like I'm missing part of the picture here... perhaps yould could comment on my example's README how to get that to actually work (and reproduce your problem) ?

rhuss commented 5 years ago

Not really remember how it works, just that the Spring Boot Devtools, which are required for hot-reload support, don't work on fat jar but only on the exploded one. 'would be interesting whether for new Spring Boot versions this is still the case.

jeffmaury commented 5 years ago

So the assemble script works that way:

  1. checks if the fat jar contains spring-devtools
  2. if true unzip the fat jar
  3. then parse the MANIFEST.MF to get the Spring Boot application class name
  4. then launch Java with specific parameters to use either the Spring Boot JAR ou WAR launcher and giving the Spring Boot application class name as argument

so step 2 is broken as it uses unzip which is not anymore in the image. Why don't we use jar for that ?

rhuss commented 5 years ago

so step 2 is broken as it uses unzip which is not anymore in the image. Why don't we use jar for that ?

good point, should have been catched in the review as the feature has been introduced.

vorburger commented 5 years ago

So the assemble script works that way:

@jeffmaury but why does my example, where the fat fat JAR does contain spring-devtools, not cause the unzip and reproduce this problem? A reproducer (example) would be a useful first step here IMHO.

jeffmaury commented 5 years ago

I supposed your sample is correct but the script is wrong as unzip is not there and you need to check the logs to check for the exploded message

vorburger commented 5 years ago

so step 2 is broken as it uses unzip which is not anymore in the image. Why don't we use jar for that ?

That's an idea, and we could do this e.g. in https://github.com/OASIS-learn-study/swissarmyknife-minecraft-server/pull/14/files, but here in this project it's not just search & replace of "unzip" by "jar xvf", because of args (-l & -p) - and I wouldn't want to raise a "blind" PR, without being able to test it (but see above; help with a reproducer test would be welcome!).

Alternatively, therefore, and IMHO safer and better for anyone else this may affect, we should just fix the regression we introduced in the move FROM jboss/base-jdk:8 to centos were we lost unzip ? I'm proposing to do this in #185.

vorburger commented 5 years ago

@jeffmaury with the just merged #185 unzip is back in this image.

Are you able to test to confirm if this issue is fixed? Using master OK, or do you need a release to Docker Hub? Do you just need a release of the upstream CentOS based community image, or also of downstream RHEL based one?

tandeday commented 5 years ago

Using Spring Boot 2.1.3.RELEASE the code works, but the test is invalid as my fat jar does not include the dev tools jar. Skipping that test and just look for the main class would in my opinion be sufficient.

vorburger commented 5 years ago

@m86194 a Pull Request with (more) fixes for this, or a working test, would probably be welcome. (FYI I'm no longer involved with this project, and just un-watched this issue.)