OSFUSE-548: Added support for spring-boot-devtools automatic restarts… #81
Conversation
… for Spring Boot images, also fixed Karaf org.jboss.deployments-dir property
There was a problem hiding this comment.
LGTM in general (but see also comments).
However personally I would move the special devtools handling to run.sh (including the extraction of a fatjar)
Not sure why the extraction of springboot fatjar is required in all cases (and not started directly from the fatjar if not used with devtools ?)
java/templates/s2i/run
Outdated
| fi | ||
| fi | ||
| # default to SpringBoot's Main-Class | ||
| if [ "x${main_class}" = x ]; then |
There was a problem hiding this comment.
If JAVA_DEBUG is not set or nor devtools present, then there is no classpath adoption for BOOT-INF/lib/* path. Is this correct ?
There was a problem hiding this comment.
The extraction makes it so that the same image can be run in debug mode by just setting the JAVA_DEBUG environment variable, and user could test non-debug execution by removing it.
SpringBoot docs make a reference to how CloudFoundry runs apps exploded, without explicitly mentioning why. I don't think we have to do the same, but left it like that in case I discovered any advantages later, which I haven't found yet. Maybe CloudFoundry does it's own hot deploy and restart or something along those lines??
I was thinking of minimizing work before launching the app. So, I am thinking the following should be done during assemble:
- devtools check to decide whether to explode the jar or not. Before digging into SB devtools and JarLauncher code I thought it could do reloads etc. without having to build a special SB jar with devtools inside, but clearly it's not possible.
- doing these tasks in assemble/run keeps run-java.sh focused on launching Java apps, instead of introducing SpringBoot cruft in it, sort of a separation of concerns.
There was a problem hiding this comment.
ok, convinced :) It's probably better to keep run-java.sh tech agnostic.
I'm not that deep in how spring devtools work actually, but I know that they need special jars prepared for working properly. Therefore we do also a new repackaging in f-m-p when we do hot deploy support for spring boot in fabric8:watch. I'm not super happy with this because it creates different artifacts (with the same 'version' number) and it might happen accidentally that devtools slip into production (e.g if there's not mvn clean somewhere inbetween).
would have to revisit the fmp code more closely to make a better statement, but I'm perfetlu fine with this PR for now.
Just lets update the README and then merge.
| @@ -30,6 +30,7 @@ The run script can be influenced by the following environment variables: | |||
| * **JAVA_APP_NAME** Name to use for the process | |||
| * **JAVA_CLASSPATH** the classpath to use. If not given, the startup script checks for a file `${JAVA_APP_DIR}/classpath` and use its content literally as classpath. If this file doesn't exists all jars in the app dir are added (`classes:${JAVA_APP_DIR}/*`). | |||
| * **JAVA_DEBUG** If set remote debugging will be switched on | |||
| * **JAVA_DEBUG_SUSPEND** If set enables suspend mode in remote debugging | |||
| * **JAVA_DEBUG_PORT** Port used for remote debugging. Default: 5005 | |||
|
|
|||
There was a problem hiding this comment.
I would add to the README a description of the changed behaviour for dealing with Spring Boot Apps.
There was a problem hiding this comment.
Agreed, I'll add some text about it to the README.
… for Spring Boot images, also fixed Karaf org.jboss.deployments-dir property