Support environment variables and system properties in configuration files #716

guwirth · 2015-12-19T10:54:34Z

introduce new class CxxSettings
overload Settings.getClearString to replace environment variables in values
replace all Settings with CxxSettings

todo: unit test

@ALL: any feedback to this approach?

jmecosta · 2015-12-19T11:11:42Z

Try to to support also vars that are passed as arguments.

For example

-DRoot=c:\src

Usually those are more useful than environment variables. Both should be coded. Suppose the you can use get clear property to retrieve the the props that are ${} before looking into the environment

guwirth · 2015-12-19T11:39:49Z

@jmecosta good idea will try. Do you know how sonar runner is handling this defines?
E.g. - DRoot=c:\src is this an environment variable afterwords or can I use an API to get them?

jmecosta · 2015-12-19T11:41:44Z

I dont think it matters, we they reached the plugin I think they are already parsed properly

jmecosta · 2015-12-19T12:00:45Z

I think you can use the settings class to get those.

guwirth · 2015-12-20T08:08:36Z

This PR is always crashing on server side.

first try was to replace all Settings with CxxSettings: server crash during start up
next try to move and use CxxSettings only internally: crash during configuration creation

@ALL someone experience with this? Is it possible to extend Settings? Is there something I have to register on server side?

jmecosta · 2015-12-20T09:18:31Z

sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/CxxSettings.java

+import java.util.Properties;
+import org.sonar.api.config.Settings;
+
+public class CxxSettings extends Settings {


wonder if the internals of Settings will be in proper state when it arrives to the plugin. i would assume core to create its own instance of Settings class, likely when CxxSettings gets into the plugin it comes without any information about the settings.

perhaps this class cannot be used like this, and might explain the issue in server side. Could this be done in a helper class that receives Settings and then returns the value as per you implementation below?

guwirth · 2015-12-23T10:12:10Z

This version is working. Maybe someone more familiar with Java can have a look to it.

jmecosta · 2015-12-23T10:25:12Z

Looks good to me, I don't know what are the java patterns that should be used in this case but for me it looks functional

guwirth · 2015-12-23T12:51:20Z

The point with this solution is, that the CxxSettings(Settings settings) Ctor creates a copy of Settings. If someone is changing the values of the original Settings afterwards this will not affect CxxSettings. I'm not sure if this is relevant (except unit tests) because after starting an analysis the Settings have to be stable anyway?

Other approach could be to use inside CxxSettings a reference and the pimpl pattern. But this means all methodes must be duplicated.

guwirth · 2015-12-24T08:56:30Z

Ready to merge 👍

Support environment variables and system properties in configuration files: use ${xxx} as placeholder

jmecosta · 2015-12-24T09:02:09Z

Let me test it first. Try to do it before end of year.

On Thu, Dec 24, 2015, 10:56 Günter Wirth [email protected] wrote:

Ready to merge [image: 👍]

Support environment variables and system properties in configuration
files: use ${xxx} as placeholder

—
Reply to this email directly or view it on GitHub
#716 (comment)
.

guwirth · 2015-12-24T13:45:24Z

Ok. Happy xmas.

Von meinem iPhone gesendet

Am 24.12.2015 um 10:02 schrieb Jorge Costa [email protected]:

Let me test it first. Try to do it before end of year.

On Thu, Dec 24, 2015, 10:56 Günter Wirth [email protected] wrote:

Ready to merge [image: 👍]

Support environment variables and system properties in configuration
files: use ${xxx} as placeholder

—
Reply to this email directly or view it on GitHub
#716 (comment)
.

—
Reply to this email directly or view it on GitHub.

jmecosta · 2015-12-24T13:54:59Z

Merry Xmas also..

On Thu, Dec 24, 2015, 15:45 Günter Wirth [email protected] wrote:

Ok. Happy xmas.

Von meinem iPhone gesendet

Am 24.12.2015 um 10:02 schrieb Jorge Costa [email protected]:

Let me test it first. Try to do it before end of year.

On Thu, Dec 24, 2015, 10:56 Günter Wirth [email protected]
wrote:

Ready to merge [image: 👍]

Support environment variables and system properties in configuration
files: use ${xxx} as placeholder

—
Reply to this email directly or view it on GitHub
<
#716 (comment)

.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#716 (comment)
.

guwirth · 2015-12-29T10:28:54Z

@jmecosta did you have some time for testing? Can I merge this?

jmecosta · 2015-12-29T10:33:01Z

i will do it today.

On Tue, 29 Dec 2015 at 12:28 Günter Wirth [email protected] wrote:

@jmecosta https://github.com/jmecosta did you have some time for
testing? Can I merge this?

—
Reply to this email directly or view it on GitHub
#716 (comment)
.

jmecosta · 2015-12-29T11:41:19Z

@guwirth few things i spotted:

if passing -DAnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults i dont get the parser to replace the AnalysisReportsDir

environment variables work, however if using
set AnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults

what i get in the plugin is:
D:SonarVersionsMultLanguageProject.cxxresults

so it does not support the windows separators.

guwirth · 2015-12-29T14:54:56Z

😕 thx for testing. Will try to improve it.

Am 29.12.2015 um 12:41 schrieb Jorge Costa [email protected]:

@guwirth few things i spotted:

if passing -DAnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults i dont get the parser to replace the AnalysisReportsDir

environment variables work, however if using
set AnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults

what i get in the plugin is:
D:SonarVersionsMultLanguageProject.cxxresults

so it does not support the windows separators.

—
Reply to this email directly or view it on GitHub.

guwirth · 2015-12-29T18:15:37Z

if passing -DAnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults i dont get the parser to replace the AnalysisReportsDir

There are three types to replace:

SQ properties from configuration file or -D parameter
Java system properties
environment variables

Currently I replace only 2 & 3. Will add also 1.

environment variables work, however if using
set AnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults
what i get in the plugin is:
D:SonarVersionsMultLanguageProject.cxxresults

In case of 2 & 3 I have to replace \ with \ or / to behave like defined in config file.

jmecosta · 2015-12-29T19:18:12Z

Ok so its possible to support the windows paths, I know that using the
sonar runner this would not work. If you are able to get it working then
would be really nice

On Tue, Dec 29, 2015, 20:15 Günter Wirth [email protected] wrote:

if passing
-DAnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults i dont
get the parser to replace the AnalysisReportsDir

There are three types to replace:

SQ properties from configuration file or -D parameter

Java system properties

environment variables

Currently I replace only 2 & 3. Will add also 1.

environment variables work, however if using
set AnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults
what i get in the plugin is:
D:SonarVersionsMultLanguageProject.cxxresults

In case of 2 & 3 I have to replace \ with \ or / to behave like defined
in config file.

—
Reply to this email directly or view it on GitHub
#716 (comment)
.

- CxxSettings clones Settings in ctor - format for placeholder is ${xxx} - supported are environment variables, Java system properties and SonarQube properties - backslashes in values from environment variables and Java system properties are replaced with slashes to support Windows paths

guwirth · 2015-12-30T07:27:18Z

-DAnalysisReportsDir=D:\SonarVersions\MultLanguageProject

For this case you have to define it like in config file. Can't replace \ with / because I don't know which properties are from config file and which are from command line.

works now:

-DAnalysisReportsDir=D:/SonarVersions/MultLanguageProject
-DAnalysisReportsDir="D:/SonarVersions/MultLanguageProject"

environment variables work, however if using set AnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults what i get in the plugin is: D:SonarVersionsMultLanguageProject.cxxresults

It's working now.

guwirth · 2015-12-30T07:28:33Z

CxxSettings supports variables in configuration files …

CxxSettings clones Settings in ctor
format for placeholder is ${xxx}
supported are environment variables, Java system properties and SonarQube properties
backslashes in values from environment variables and Java system properties are replaced with slashes to support Windows paths

jmecosta · 2015-12-30T07:30:08Z

Ok I will test again in a bit. Thanks

On Wed, Dec 30, 2015, 09:27 Günter Wirth [email protected] wrote:

-DAnalysisReportsDir=D:\SonarVersions\MultLanguageProject

For this case you have to define it like in config file. Can't replace
with / because I don't know which properties are from config file and which
are from command line.

works now:

-DAnalysisReportsDir=D:/SonarVersions/MultLanguageProject
-DAnalysisReportsDir="D:/SonarVersions/MultLanguageProject"

environment variables work, however if using set
AnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults what i
get in the plugin is: D:SonarVersionsMultLanguageProject.cxxresults

It's working now.

—
Reply to this email directly or view it on GitHub
#716 (comment)
.

jmecosta · 2015-12-30T16:06:07Z

works fine now, thanks. for me it can be merged

On Wed, 30 Dec 2015 at 09:29 Jorge Costa [email protected] wrote:

Ok I will test again in a bit. Thanks

On Wed, Dec 30, 2015, 09:27 Günter Wirth [email protected] wrote:

-DAnalysisReportsDir=D:\SonarVersions\MultLanguageProject

For this case you have to define it like in config file. Can't replace
with / because I don't know which properties are from config file and which
are from command line.

works now:

-DAnalysisReportsDir=D:/SonarVersions/MultLanguageProject
-DAnalysisReportsDir="D:/SonarVersions/MultLanguageProject"

environment variables work, however if using set
AnalysisReportsDir=D:\SonarVersions\MultLanguageProject.cxxresults what i
get in the plugin is: D:SonarVersionsMultLanguageProject.cxxresults

It's working now.

—
Reply to this email directly or view it on GitHub
#716 (comment)
.

Support environment variables and system properties in configuration files

guwirth added the enhancement label Dec 19, 2015

guwirth added this to the M 0.9.5 milestone Dec 19, 2015

guwirth self-assigned this Dec 19, 2015

jmecosta closed this Dec 19, 2015

jmecosta reopened this Dec 19, 2015

jmecosta reviewed Dec 20, 2015
View reviewed changes

guwirth changed the title ~~Support environment variables in configuration files~~ Support environment variables and system properties in configuration files Dec 24, 2015

guwirth added the docu request label Dec 30, 2015

guwirth added a commit that referenced this pull request Dec 30, 2015

Merge pull request #716 from guwirth/enhancement/environment-variables

0bd42d2

Support environment variables and system properties in configuration files

guwirth merged commit 0bd42d2 into SonarOpenCommunity:master Dec 30, 2015

guwirth mentioned this pull request Jan 5, 2016

improve variables support in configuration file #741

Merged

guwirth mentioned this pull request Feb 8, 2016

environment variables are not working in multi-module projects #773

Closed

guwirth deleted the enhancement/environment-variables branch April 3, 2016 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support environment variables and system properties in configuration files #716

Support environment variables and system properties in configuration files #716

guwirth commented Dec 19, 2015

jmecosta commented Dec 19, 2015

guwirth commented Dec 19, 2015

jmecosta commented Dec 19, 2015

jmecosta commented Dec 19, 2015

guwirth commented Dec 20, 2015

jmecosta Dec 20, 2015

guwirth commented Dec 23, 2015

jmecosta commented Dec 23, 2015

guwirth commented Dec 23, 2015

guwirth commented Dec 24, 2015

jmecosta commented Dec 24, 2015

guwirth commented Dec 24, 2015

jmecosta commented Dec 24, 2015

guwirth commented Dec 29, 2015

jmecosta commented Dec 29, 2015

jmecosta commented Dec 29, 2015

guwirth commented Dec 29, 2015

guwirth commented Dec 29, 2015

jmecosta commented Dec 29, 2015

guwirth commented Dec 30, 2015

guwirth commented Dec 30, 2015

jmecosta commented Dec 30, 2015

jmecosta commented Dec 30, 2015

Support environment variables and system properties in configuration files #716

Support environment variables and system properties in configuration files #716

Conversation

guwirth commented Dec 19, 2015

jmecosta commented Dec 19, 2015

guwirth commented Dec 19, 2015

jmecosta commented Dec 19, 2015

jmecosta commented Dec 19, 2015

guwirth commented Dec 20, 2015

jmecosta Dec 20, 2015

Choose a reason for hiding this comment

guwirth commented Dec 23, 2015

jmecosta commented Dec 23, 2015

guwirth commented Dec 23, 2015

guwirth commented Dec 24, 2015

jmecosta commented Dec 24, 2015

guwirth commented Dec 24, 2015

jmecosta commented Dec 24, 2015

guwirth commented Dec 29, 2015

jmecosta commented Dec 29, 2015

jmecosta commented Dec 29, 2015

guwirth commented Dec 29, 2015

guwirth commented Dec 29, 2015

jmecosta commented Dec 29, 2015

guwirth commented Dec 30, 2015

guwirth commented Dec 30, 2015

jmecosta commented Dec 30, 2015

jmecosta commented Dec 30, 2015