-
Notifications
You must be signed in to change notification settings - Fork 657
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
Add environment variable support for user-mgt configuration #2343
base: 4.5.x
Are you sure you want to change the base?
Conversation
@@ -126,4 +126,32 @@ public static String getCarbonHome() { | |||
} | |||
return carbonHome; | |||
} | |||
|
|||
public static String replaceSystemProperty(String text) { |
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.
Add a java doc comment.
int indexOfStartingChars = -1; | ||
int indexOfClosingBrace; | ||
|
||
// The following condition deals with properties. |
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.
Multiline comments should be used if the comment spans over a single line.
|
||
// The following condition deals with properties. | ||
// Properties are specified as ${system.property}, | ||
// and are assumed to be System properties |
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.
Each sentence must conclude with an apostrophe.
@@ -181,6 +182,9 @@ private static void addPropertyElements(OMFactory factory, OMElement parent, Str | |||
Map.Entry<String, String> entry = ite.next(); | |||
String name = entry.getKey(); | |||
String value = entry.getValue(); | |||
if (value != null) { |
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.
Abstract the null check inside the replaceSystemProperty() method.
Refactor the other two usages of this method accordingly.
// and are assumed to be System properties | ||
while (indexOfStartingChars < text.indexOf("${") | ||
&& (indexOfStartingChars = text.indexOf("${")) != -1 | ||
&& (indexOfClosingBrace = text.indexOf('}')) != -1) { |
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.
Can't we validate the pattern of "${system.property}" using a regex?
Also, what is the reason to use a while loop? Can't we replace it with an if condition after matching the pattern?
while (indexOfStartingChars < text.indexOf("${") | ||
&& (indexOfStartingChars = text.indexOf("${")) != -1 | ||
&& (indexOfClosingBrace = text.indexOf('}')) != -1) { | ||
String sysProp = text.substring(indexOfStartingChars + 2, |
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.
This can also be handled using a regex pattern which will improve the readability.
&& (indexOfClosingBrace = text.indexOf('}')) != -1) { | ||
String sysProp = text.substring(indexOfStartingChars + 2, | ||
indexOfClosingBrace); | ||
String propValue = System.getProperty(sysProp); |
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.
Line 142-146 can be simplified using Optionals.
E.g. Optional.orElseGet()
propValue = System.getenv(sysProp); | ||
} | ||
if (propValue != null) { | ||
text = text.substring(0, indexOfStartingChars) + propValue |
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.
Use a StringBuilder to avoid multiple concatenations.
} | ||
if (sysProp.equals("carbon.home") && propValue != null | ||
&& propValue.equals(".")) { | ||
text = new File(".").getAbsolutePath() + File.separator + text; |
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.
Use a StringBuilder to avoid multiple concatenations.
text = text.substring(0, indexOfStartingChars) + propValue | ||
+ text.substring(indexOfClosingBrace + 1); | ||
} | ||
if (sysProp.equals("carbon.home") && propValue != null |
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.
Use a constant for "carbon.home"
+ text.substring(indexOfClosingBrace + 1); | ||
} | ||
if (sysProp.equals("carbon.home") && propValue != null | ||
&& propValue.equals(".")) { |
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.
Use StringUtils.equals to avoid the explicit null check of "propValue".
Purpose
Fix wso2/product-ei#4724