Spring-boot: Add inlined conf file support to launch.script

[philwebb]

https://github.com/spring-projects/spring-boot/commit/ba2ea301f1cc7df7104293bcdf567416c1fda4bb#commitcomment-10652388

[aantono]

Also, a neat trick that we are using, is to embed the jvm.options file into the archive itself. We place it into src/resources/jvm.options location as a text file and later extract the content in the bash script like this:

jvm_opts=`unzip -p $jarfile jvm.options 2>/dev/null`
echo "Starting application using the following jvm.options: $jvm_opts"

That gets also added to the startup command line. This way applications can have an ability to ship/store the static pre-defined JVM system properties in their code/bundle and store them in the version control. This is very helpful for configuring XML parsers, JodaTime TZ handlers and other things of that nature, that are driven via -D arguments.

[philwebb]

The build plugins now support a embeddedLaunchScriptProperties which can be used as a general purpose mechanism to replace parts of the init script. I was thinking we could just add something like we already have for mode. The mode flag also allows overrides when running the app:

$ mode="service" ./myapp.jar

Something similar for JAVA_OPTS would allow local overrides:

$ JAVA_OPTS="-X..." ./myapp.jar

[aantono]

That sounds like a good way to handle...
The only reason we embed jvm.options in the jar, is for the never-changing static configuration. We use a specific instance of javax.xml.parsers.DocumentBuilderFactory, javax.xml.parsers.SAXParserFactory, org.joda.time.DateTimeZone.Provider and some other things. Those are not environment specific, just different from the standard configuration of those libraries or settings.

For us the rule of thumb is: if it varies from env to env, like memory footprint, set it via JAVA_OPTS variable, if it is static, put those props into jvm.options in the build.

[philwebb]

Perhaps we need a couple of variables in the script? One for static options and one for overrides. BTW you can also use embeddedLaunchScript in your build file to embed a completely custom script.

[aantono]

Yeah, that probably would be a better option. This way the static startup system properties can be defined in one place in the build.gradle or pom.xml build file and then used for all the places where the application is started, like bootRun task, and for generating the self-executable and being transformed into the embeddedLaunchScriptProperties which will be placed into the launch script {{static_jvm_ops}} template variable, as opposed to JAVA_OPTS externally set ENV variable.

[davidrabinowitz]

@philwebb Please notice that in many cases setting the environment variables is not done by the developers but by the deployer/devops/etc by manipulating the environment variables and the command line arguments.

It would be helpful to have two environment variables like tomcat has - JAVA_OPTS for start and stop, and another (SPRING_BOOT_OPTS?) only for start - similar to CATALINA_OPTS

[joshiste]

Isn't this issue obsolete after f0bb60bf9d4b4b72ccdfda4c530d8a4b000e2047?

[philwebb]

Thanks to @joshiste this should be sorted now.

[JustinKSU]

@philwebb pull request #3243 did allow you to create a conf file the same name as the .jar file to pass JVM parameters. However, this has a few downsides. Every time you build a new version you need to create a new .conf file with the same version in the filename. Also there is no way to bundle the JVM parameters with your Maven artifact.

I think adding "javaOpts" to embeddedLaunchScriptProperties would be powerful. It would be important that this change only append JAVA_OPTS not replace those defined in the environment or by the .conf file.

A single executable .jar file with JVM options built-in allows for simpler deployments. Would you be willing to re-open this issue so I could do a pull request against it?

[wilkinsona]

It would be important that this change only append JAVA_OPTS not replace those defined in the environment or by the .conf file

Can you please explain why you think this behaviour is important?

Appending to the options in the environment or the .conf file sounds a little limited to me. My main concern is that it means that you can only add to the options that are baked into the jar file and there's no way that you can replace them entirely.

[wilkinsona]

Every time you build a new version you need to create a new .conf file with the same version in the filename.

Your build system can take care of this for you

Also there is no way to bundle the JVM parameters with your Maven artifact.

Your Maven artifact could be a zip that contains the jar and the .conf file

[JustinKSU]

@wilkinsona I put my responses to your comments in #9375

[JustinKSU]

Appending to the options in the environment or the .conf file sounds a little limited to me. My main concern is that it means that you can only add to the options that are baked into the jar file and there's no way that you can replace them entirely.

Good point. It would be better to prepend them. Then they could be overridden. The JVM takes whatever the last value in the argument list for a given key.

[wilkinsona]

Following some discussion in #9375, we'd like to provide something that doesn't just target JAVA_OPTS but offers a more general purpose solution. The plan is to allow a conf file to be inlined into the default launch script at build time. From #9375:

If we inlined the file here, I think it would then allow an external .conf file to change whatever the inlined file has set up. For example it could change JAVA_OPTS to whatever it wanted, prepending, appending, or replacing entirely as required. It'd also be analogous to application.properties with an external .conf file taking precedence over the one that's been inlined into the launch script.

[wilkinsona]

Some questions from @JustinKSU that are probably best answered here:

All, If we went with allowing a ".conf" file to be included in the project, would we add "embeddedLaunchConfig" or "embeddedLaunchConf" at the same level as "embeddedLaunchScript"? Or would you see it as another embeddedLaunchScriptProperties?

Inlining the file is a feature of the embedded launch script, so it doesn't feel to me like it should be at the same level as embeddedLaunchScript. Perhaps it could be another property that's given special treatment. The property's value could be a File or a String that is a path to a file. DefaultLaunchScript could then read in this file and update the property's value to be the file's contents. The inlining can then happen using the standard templating support that DefaultLaunchScript already provides.

[JustinKSU]

The property's value could be a File or a String that is a path to a file.

Do you have a reference on how you can put a File in a Maven configuration? I have always seem relative paths as strings e.g. src/main/resources/somefile.txt.

Not sure if this is a good idea, but you could allow the user to specify a single line to get injected like -Dfoo=bar or classpath:path/to/script.txt if they want to import a file.

[wilkinsona]

I think Maven automatically coerces the string to a file. We use a File for a custom launch script in RepackageMojo.

I'm not keen on letting users write the inlined conf file's contents directly in their pom.xml or build.gradle. It feels like we'd be encouraging people to do something daft and try and specify a long multi-line script that way.

Using classpath: to locate the file feels a bit odd as you probably don't want the file to be on the classpath. If it were, it would be packaged in the jar unless you filtered it out.

The more I think about this the more that mirroring what's supported for specifying a custom script feels right to me.

[JustinKSU]

Sounds good. So basically you can replace the entire startup script, or keep the template and add the "extra" sauce you need for your project in the middle.

[jovi521]

OK

[JustinKSU]

Any suggestion on what to call the option that references the file that should be inlined? Some thoughts, none of which I'm excited about:

• inlinedScript
• additionalCommands
• subScript
• preStartupScript

[wilkinsona]

Naming things is always hard… I think it'd be good to use conf somewhere in the name to tie it to the external conf file:

• inlinedConfFile
• inlinedConfScript

We already have the embeddedLaunchScript property so, of those two, I think I prefer inlinedConfScript.

[JustinKSU]

@wilkinsona you suggested that inlinedConfScriptshould be a new embeddedLaunchScriptPropertiesproperty. However, Maven property values are Strings. They are not automatically coerced to Files if the string happens to match the path to a file. The easiest thing to do is have inlinedConfScriptbe at the same level as embeddedLaunchScript as a Fileparameter. However, if you feel it's important that it be a embeddedLaunchScriptProperties, I can add special logic to parse the String and load the corresponding file.

Personally I think inlinedConfScriptbeing at the same level as embeddedLaunchScriptis actually a good thing. Together in the documentation you can see the options you have of replacing or enhancing the script. However, I understand the "natural" fit of it being a property of the script itself.

What does the community think?

[wilkinsona]

They are not automatically coerced to Files if the string happens to match the path to a file

I don't think that matters. It can be easily turned into a File with new File(propertyValue)

if you feel it's important that it be a embeddedLaunchScriptProperties

I do. For the inlined conf script to have any effect, the proposal is that it will rely on variable replacement in the embedded script. That's done using the embeddedLaunchScriptProperties.

[JustinKSU]

I've finished coding and am working on unit tests. I noticed there is no unit test for the RepackageMojo. I want to test to make sure that I'm correctly converting a referenced file into it's contents. Should I be using something like maven-testing-harness? Instead should I change the method I want to test to "package default" or use reflection to call it? Alternatively I could mock all the member objects of RepackageMojo and use reflection to set them, but that seems like not the right way to test a Maven plugin.

Thanks for your guidance and helping me through my first Spring contribution.

[wilkinsona]

I noticed there is no unit test for the RepackageMojo. I want to test to make sure that I'm correctly converting a referenced file into it's contents

I would push that logic down into DefaultLaunchScript. It'll make it easier to test and will also allow the Gradle plugin to reuse the same logic. Note that a small API change will be required to the Gradle plugin as LaunchScriptConfiguration currently uses a Map<String, String> for the properties. I think this should change to Map<String, Object>.

[JustinKSU]

If the objective is to keep it consistent and push the logic into the DefaultLaunchScript, then it would need to stay Map<String, String> as only the path to the file would be passed in via the Maven plugin. If Gradle makes it easier to reference a File object, and we want to support referencing a File object instead of just the path, then it makes more sense to change the Maven Plugin convert the String reference to a File object before passing it into the DefaultLaunchScript. If that's the case, then my questions about how to write unit tests for Spring's Maven plugin still exist.

It seems to me that we should be consistent between the two plugins and assume what's in the configuration file is a reference to the path, not a File object. I'm not familiar with Gradle, but it seems that supporting a File reference in Gradle and only the String path in the Maven plugin could cause confusion. I look to your expertise on what is "normal" for a Gradle user.

Whether we read the file contents of before calling DefaultLaunchScript or inside DefaultLaunchScript seems subjective to me. In my opinion, it seems that the DefaultLaunchScript shouldn't care where the String values it's replacing came from. Currently it's not aware of any of the variable names and I think it make sense to keep it that way. That's why I made the choice to read the file in the Maven plugin, so that the DefaultLaunchScript would still be generic and not have to change. I could do the same in the Gradle plugin, or have a common util method that takes the properties file and replaces the paths with the file's contents before it's sent to the DefaultLaunchScript constructor.

[JustinKSU]

If you are OK with hard coding variable names in DefaultLaunchScript class, I can have it check the object type and coerce String objects into File objects. I believe that is what you initially described. If so, I can move forward with that design.

[wilkinsona]

If Gradle makes it easier to reference a File object, and we want to support referencing a File object instead of just the path, then it makes more sense to change the Maven Plugin convert the String reference to a File object before passing it into the DefaultLaunchScript

It's trivial to reference a File in Gradle, and I think we should support it. However, it's also trivial to reference a path as a String in Gradle and I think we should support that too. Keeping the conversion in common code that's shared between the two plugins continues to make the most sense to me.

It seems to me that we should be consistent between the two plugins and assume what's in the configuration file is a reference to the path, not a File object. I'm not familiar with Gradle, but it seems that supporting a File reference in Gradle and only the String path in the Maven plugin could cause confusion. I look to your expertise on what is "normal" for a Gradle user.

Maven users are used to everything being strings in XML, Gradle users are used to having every type under the sun available to them. We should embrace those differences and support what makes sense to users of the two different build systems.

Currently it's not aware of any of the variable names and I think it make sense to keep it that way

DefaultLaunchScript is very tightly coupled to the default launch.script file where all of the variable names are hardcoded. Whether that hardcoding is in the default template or in Java code doesn't make much difference.

I could do the same in the Gradle plugin, or have a common util method that takes the properties file and replaces the paths with the file's contents before it's sent to the DefaultLaunchScript constructor.

Given that we already have code that's shared between the two plugins, duplicating the logic doesn't sound like a good option to me. As for where it goes in the shared code, keeping the logic in DefaultLaunchScript still appeals to me as it has the advantage that its callers will get the enhanced behaviour with having to do anything, i.e. they won't have to call a utility class to do the property preprocessing.

If you are OK with hard coding variable names in DefaultLaunchScript class, I can have it check the object type and coerce String objects into File objects. I believe that is what you initially described. If so, I can move forward with that design.

Thanks. That does feel like the best option to me, although it's hard to be certain without seeing how it looks once it's in code…

[JustinKSU]

@wilkinsona

Here is what I'm thinking. If you like it I'll add unit tests and put an official pull request: https://github.com/JustinKSU/spring-boot/commit/ece6f7444e5fa2bbda8a9cb8de57307977d26788

Feedback appreciated.

[wilkinsona]

Thanks, @JustinKSU. It looks good to me. A couple of minor points:

• Please use Spring Framework's FileCopyUtils.copyToString(Reader) method, rather than Commons IO
• I'd isolate the new logic to only those keys that support a file and fail-fast if the value is not a file, is not a string that can be coerced to a file, or the file does not exist

[JustinKSU]

@wilkinsona
Feel a lot better about this version. Let me know what you think: https://github.com/JustinKSU/spring-boot/commit/4cf1ff9edfea393bd5aef4140008aa758ace15b6

[wilkinsona]

@JustinKSU Looks good to me. Would you like to turn it into a PR please?

[JustinKSU]

Thank you @wilkinsona for your extra attention on such a small change. Pull request #9590 has been created.

[snicoll]

Closing in favour of PR #9590

[wilkinsona]

Thank you @wilkinsona for your extra attention on such a small change.

Not at all. Thank you for the discussion and your interest in contributing.

Pull request #9590 has been created.

Thanks very much.

[liuhuagui]

How to use inlinedConfScript to configure JAVA_OPTS with maven? Hope to get your help @JustinKSU ，Thanks.