Re: Review Request: Suggested Fix for JIRA Issue OODT-553
On Jan. 30, 2013, 2:09 a.m., Chris Mattmann wrote: Michael Starch wrote: The line line = line.replaceAll(, ); is used to allow the call Properties.load(InputStream). The reason for this is the load command expects '\' to escape something, and will drop invalid escapes. Therefore '\' must become \\ to be properly loaded without causing unwanted escapes or being ignored. This is all handled by java on the back end when calling getEnvironment i.e. any env variable, complete with any '\'s is already properly read in. I cannot get the new method to fail on any environment variable containing series of 1 to 7 '\'s against the old implementation in a regression test. Therefore, I expect the above implementation to function properly. Thanks Mike OK that makes sense to me. I'll go ahead and apply your patch thank you! - Chris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/#review15817 --- On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/ --- (Updated Jan. 29, 2013, 9:25 p.m.) Review request for oodt and Chris Mattmann. Description --- This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec env. Diffs - /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711 /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711 Diff: https://reviews.apache.org/r/9142/diff/ Testing --- Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set. Thanks, Michael Starch
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/#review16042 --- Ship it! Ship It! - Chris Mattmann On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/ --- (Updated Jan. 29, 2013, 9:25 p.m.) Review request for oodt and Chris Mattmann. Description --- This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec env. Diffs - /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711 /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711 Diff: https://reviews.apache.org/r/9142/diff/ Testing --- Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set. Thanks, Michael Starch
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
On Jan. 30, 2013, 2:09 a.m., Chris Mattmann wrote: /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java, line 77 https://reviews.apache.org/r/9142/diff/1/?file=253029#file253029line77 Hey Mike, here we are no longer calling preProcessInputStream -- doesn't that do envVarReplace? Chris, the only thing that this function does is line = line.replaceAll(, );. There is no envVarReplace here. I did some quick tests to see if this was an issue, but I will test this again to see if I can get a better idea of any implications here. Are you experiencing any specific problems? - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/#review15817 --- On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/ --- (Updated Jan. 29, 2013, 9:25 p.m.) Review request for oodt and Chris Mattmann. Description --- This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec env. Diffs - /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711 /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711 Diff: https://reviews.apache.org/r/9142/diff/ Testing --- Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set. Thanks, Michael Starch
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
On Jan. 30, 2013, 2:09 a.m., Chris Mattmann wrote: The line line = line.replaceAll(, ); is used to allow the call Properties.load(InputStream). The reason for this is the load command expects '\' to escape something, and will drop invalid escapes. Therefore '\' must become \\ to be properly loaded without causing unwanted escapes or being ignored. This is all handled by java on the back end when calling getEnvironment i.e. any env variable, complete with any '\'s is already properly read in. I cannot get the new method to fail on any environment variable containing series of 1 to 7 '\'s against the old implementation in a regression test. Therefore, I expect the above implementation to function properly. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/#review15817 --- On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/ --- (Updated Jan. 29, 2013, 9:25 p.m.) Review request for oodt and Chris Mattmann. Description --- This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec env. Diffs - /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711 /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711 Diff: https://reviews.apache.org/r/9142/diff/ Testing --- Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set. Thanks, Michael Starch
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/#review15817 --- /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java https://reviews.apache.org/r/9142/#comment34072 Hey Mike, here we are no longer calling preProcessInputStream -- doesn't that do envVarReplace? - Chris Mattmann On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9142/ --- (Updated Jan. 29, 2013, 9:25 p.m.) Review request for oodt and Chris Mattmann. Description --- This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec env. Diffs - /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711 /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711 Diff: https://reviews.apache.org/r/9142/diff/ Testing --- Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set. Thanks, Michael Starch
