Skip to content
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

Allow to substitute key/value using a system property, otherwise use the default value for the server.port #986

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cmoulliard
Copy link
Contributor

This fix allows to resolve the issue reported within #880

/**
     *  Look within all the System Properties if there is a property which corresponds to the key passed
     *  If a match exists, then replace the value otherwise use the default value defined within ${key:default-value}
     *  Example
     *  my.server.port=7777 is a system property that we will search in order to replace its value within the key passed to the method.
     *  If the key is "server.port: ${my.server.port:6666}", then "my.server.port" value will be substituted, otherwise the default value is used
     *  "6666"
     *
     *  @param key The string containing the key to search
     *  @return String The modified key
     */

@cmoulliard
Copy link
Contributor Author

cmoulliard commented Jul 17, 2017

Can somebody has a look to please, .... ? It will be used at least for Brewery project

@nicolaferraro
Copy link
Member

Hi @cmoulliard, I'll look at it soon.

Copy link
Member

@nicolaferraro nicolaferraro left a 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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

@cmoulliard
Copy link
Contributor Author

build time system properties?

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

application1.yaml
server.port: ${PORT:9990}
spring.rabbitmq.host: ${RABBIT_HOST:localhost}

application2.yaml
server.port: ${PORT:9991}
spring.rabbitmq.host: ${RABBIT_HOST:localhost}
...

When I will start locally the Application1 or Application2 using the following mvn spring-boot:run, it is not required to change the port number or the address of the Rabbit Messaging broker.

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 JAVA_OPTIONS=-DPORT=8080

For the other key, it will be required that we specify the DNS Name of the Rabbit instance deployed to rabbitmq to access it.

@kameshsampath
Copy link
Contributor

@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 ?

@nicolaferraro
Copy link
Member

Well, I asked to move the logic of property resolution to SpringBootUtils because I think the syntax/semantics of application.properties is Spring-Boot specific, so it can be reused in other parts of the plugin that need to read the configuration.

E.g. the ${prop:default} syntax is valid within spring-boot, but each property parsing lib (XProperty, Common Config, ...) can use a different syntax afair.

Dynamic configuration of the port using system/maven properties is already available for JavaExecGenerator (and also SpringBootGenerator) using the webPort configuration parameter of the generator.

@rohanKanojia rohanKanojia added the target/3.5 PR targeted to 3.5 label Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target/3.5 PR targeted to 3.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants