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

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

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

gilPts
Copy link
Contributor

@gilPts gilPts commented Nov 25, 2021

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)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

@ieugen
Copy link
Member

ieugen commented Dec 29, 2021

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

@ieugen
Copy link
Member

ieugen commented Dec 30, 2021

@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
Copy link
Member

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
Copy link
Member

ieugen commented Dec 30, 2021

@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
Copy link
Member

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
Copy link
Contributor

mbrohl commented Dec 31, 2021

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

@ieugen
Copy link
Member

ieugen commented Dec 31, 2021

@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
Copy link
Contributor

mbrohl commented Dec 31, 2021

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
Copy link
Member

ieugen commented Jan 2, 2022

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
Copy link
Contributor Author

gilPts commented Jan 3, 2022

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
Copy link
Contributor

mbrohl commented Jan 3, 2022

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
Copy link
Contributor Author

gilPts commented Jan 3, 2022

@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
Copy link
Contributor

mbrohl commented Jan 3, 2022

@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
Copy link
Contributor Author

gilPts commented Jan 3, 2022

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

gilPts added 2 commits January 3, 2022 16:48
…guration

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)
…guration

This commit add unit test and documentation regarding the improvement

Thanks Eugen for the review
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@JacquesLeRoux
Copy link
Contributor

Hi, what is the status here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants