Implemented: getEnvironmentProperty to allow environment variable configuration (OFBIZ-9498)

[gilPts]

Implemented: getEnvironmentProperty to allow environment variable configuration (OFBIZ-9498)

This will enable configuration of one OFBiz instance without modifying
source code, using environment variables.
Available in :

• property files
• serviceengine.xml
• entityengine.xml
• build.gradle (native)

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.2% Duplication

[ieugen]

I will make some time these days to check the PR and add feedback on it.

[ieugen]

@gilPts : Here is my feedback:

The PR looks good overall.

• I would add documentation for this feature in the asciidoc files + link to issue ?!
• I would make the code easier to test and add tests for these cases.

Make it easier to test by passing the env map https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#getenv() .
public static Map<String,String> getenv() .

Add extra method:

 public static String getEnvironmentProperty(Map<String,String> env, String value) { 
  // Move the implementation here
 }

// Update old method to delegate to the other one like this
 public static String getEnvironmentProperty(String value) { 
     return getEnvironmentProperty(System.getenv(), value); 
 }

Write tests that pass whatever environment you want.

[PierreSmits]

I like this improvement. But it seems to me that there might be more properties that could be applicable (not those related to run-time configuration, which can be retrieved from the SystemProperties table).

[ieugen]

@PierreSmits : True, and maybe we should consider moving those there.
I don't think we will have a lot of config options use via ENV.
For those that need this configuration - I don't think they can be configured via DB:
You can't configure db connection strings from the DB, JVM options as well, perhaps even ports.

I did not saw a guide for how to best handle OFBiz configuration.

[PierreSmits]

Indeed, Ioan.
Any property value used during the startup of OFBiz can't be set as a SystemProperty record.

I guess that applies to most in the config folders of the framework (folder) components. But still there are several there that should be considered as run-time configurations. USD comes to mind, as one example.

[mbrohl]

@gilPts with this change, are users forced to use environment variables? Or are the changes in entityengine.xml just for demonstration purposes?

[ieugen]

@mbrohl : ENV is opt-in.
If you check the code, you will see default vaules are provided.
End user does not have to do anything, but can.

 jdbc-uri="${env:OFB_POSTGRES_DB:jdbc:postgresql://127.0.0.1/ofbiz}"
                jdbc-username="${env:OFB_POSTGRES_USER:ofbiz}"
                jdbc-password="${env:OFB_POSTGRES_PASS:ofbiz}"

[mbrohl]

I've seen the default values , thanks @ieugen .

I'll add some input here in the next coming days, let's see if we can combine this with a centralized configuration approach.

[ieugen]

Happy new year!
@mbrohl : Centralized configuration would be very nice. But I would prefer not to drag this for too long.
As it is, the PR is pretty small and easy to work with.
If you have any suggestions that could be fitted here it might make sense to wait.

Otherwise, keeping it in small chunks that follow a plan and getting feedback along the way is the best way IMO.
@PierreSmits suggestion with configuration in DB is quite good IMO.
This is ESP true since plugins could migrate that configuration automatically at one point - but that is another discussion.

[gilPts]

Hello @ieugen,
I will take your remarks and update the pr soon, thanks.

@mbrohl indeed, the idea is to make it optional, but defining variable name in entityengine and other place is needed to ease configuration via env variable.

Like i explained in : https://issues.apache.org/jira/browse/OFBIZ-9498?focusedCommentId=17466789&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17466789

There soon will be a proposal from Marco that will introduce a centralized configuration management, with overload config mechanism. We just need to le him some time :).

[mbrohl]

Hi @gilPts ,
we already have such a centralized configuration mechanism in production for several years now. Maybe we should share this and let him check if it fits the needs, why do the work again?

[gilPts]

@mbrohl , when I say he will introduce it, it means that he will explain the needs, and the improvements it will produce. Again let's wait for him to be ready, there is no hurry.

This PR, can advance in the meantime, implementing Eugen suggestions.

[mbrohl]

@mbrohl indeed, the idea is to make it optional, but defining variable name in entityengine and other place is needed to ease configuration via env variable.

As long as this is optional and users can still use the property based configuration as usual I am fine with this PR.
Along with the already existing SystemProperty configuration option it gives the users more options to handle their configurations without forcing them to change the existing mechanism.

[gilPts]

@mbrohl Yes nothing is enforced, the only impact for existing implementation is having the ${env: annotation in some config files, those can be replaced (or only the default value to be replace), like it's need to be done nowadays with the annotation.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[JacquesLeRoux]

Hi, what is the status here?