-
Notifications
You must be signed in to change notification settings - Fork 202
Allow to substitute key/value using a system property, otherwise use the default value for the server.port #986
base: master
Are you sure you want to change the base?
Conversation
…the default value
Can somebody has a look to please, .... ? It will be used at least for Brewery project |
Hi @cmoulliard, I'll look at it soon. |
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.
From a semantic point of view, you know that property resolution in spring-boot is not limited to system properties, but also env variables (with relaxed binding) and other properties (i.e. a property can reference another property, which reference another property, which...). With or without default value declared after ":".
This change allows to use the value in system properties for server.port
only. I think that once we extend property resolution, we should do it for other parts of the plugin as well.
I'm not sure if we should consider (local) system properties for resolution, since properties that you have at build time will be different from the ones available at run time inside Kubernetes. So if I run the plugin with -Dport=9999
, the plugin would build a pod exposing port 9999
(enriched with a service), but the spring-boot application will be bound to port 8080 inside the container (because system property is not added to the container).
Adding support for default values is ok. I'd rather use other properties in application.properties
or also resources/env in the plugin configuration for resolving a property value.
Is there a specific use case for using build time system properties?
* @param key The string containing the key to search | ||
* @return String The modified key | ||
*/ | ||
protected static String replaceKeyWithSystemPropertyValue(String key) { |
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.
Wouldn't it make sense to respect Maven properties, too ? They would have been replaced if used without default value, but with default value this (e.g. with the ':') this is not recognized by maven as a property. So I would add project.getProperties() to the argument list, 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.
As Nicola proposed also that we support maven properties and system properties where maven props override system props, I will update the PR accordingly.
Nicola suggested too to move the code of the method replaceKeyWithSystemPropertyValue
to the class SpringBootUtil packaged within f-m-p core. Do you agree too @nicolaferraro @rhuss ?
This is typically the case when you have a project that you can run locally or deployed on openshift/cloudfoundry. If the application contains many spring boot servers to be started locally, then it will be required to specify different port numbers like also the HOSTNAME of the services to be accessed
When I will start locally the Application1 or Application2 using the following Of course when the same project is build/deployed on OpenShift, it could be required to change the PORT and that will impose that we do 2 modifications; 1) First to pass a maven property or system property used by f-m-p to avoid the NumberException and 2) We add it to the JAVA_OPTIONS env key defined for the DeploymentConfig For the other key, it will be required that we specify the DNS Name of the Rabbit instance deployed to |
@nicolaferraro / @rhuss / @cmoulliard - I think we can make this change in common JavaExecGeneator as then this can be re used by other frameworks as well. Doing in SpringBootGen alone will make it it spring boot specific .. thoughts ? |
Well, I asked to move the logic of property resolution to E.g. the Dynamic configuration of the port using system/maven properties is already available for JavaExecGenerator (and also SpringBootGenerator) using the |
This fix allows to resolve the issue reported within #880