You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Roman Shaposhnik <rv...@apache.org> on 2015/05/29 17:31:12 UTC
Review Request 34814: GEODE-38. Gfsh init script ignored
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34814/
-----------------------------------------------------------
Review request for geode.
Repository: geode
Description
-------
Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
>From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
"
#
Copy default .gfshrc to the home directory. Uncomment if needed.
#
#if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
#fi
"
If this file is specified, it is currently ignored.
Diffs
-----
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/34814/diff/
Testing
-------
Thanks,
Roman Shaposhnik
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Kirk Lund <kl...@pivotal.io>.
1) System Rules for JUnit 4
I really like System Rules for JUnit 4. This library has been added to our
build on GitLabs develop but not in ASF git yet
http://stefanbirkner.github.io/system-rules/. This line resets all System
Properties changes you make in your tests:
@Rule
public final RestoreSystemProperties restoreSystemProperties = new
RestoreSystemProperties();
I think you could add it to open/build.gradle in ASF git if you want to use
these Rules before our next code merge (just add one line above the junit
4.11 dependency line):
testCompile 'com.github.stefanbirkner:system-rules:1.9.0'
testCompile 'junit:junit:4.11'
Or obviously you can write an @After/@AfterClass method to reset any System
Properties you set.
2) Unit test class names
I think our gradle test target requires the tests to be named *JUnitTest
for now: GfshConfigInitFileJUnitTest, GfshInitFileJUnitTest. Or we'll have
to change the gradle config to execute all Tests without "DUnit" in the
name for "test" and "integrationTest" (currently the latter targets look
for *JUnitTest).
3) Code format
Also, 2-space soft tabs is our official standard for indentation.
Changes look good! I think we should look into creating some reusable Rules
for all that logging setup you had to write in the tests.
On Fri, May 29, 2015 at 9:01 AM, Bruce Schuchardt <bs...@pivotal.io>
wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34814/#review85742
> -----------------------------------------------------------
>
>
> This looks good, with the exception that I think there needs to be an
> @After in the config file unit test that resets the system properties to
> what they were before the test ran.
>
> - Bruce Schuchardt
>
>
> On May 29, 2015, 3:39 p.m., Roman Shaposhnik wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/34814/
> > -----------------------------------------------------------
> >
> > (Updated May 29, 2015, 3:39 p.m.)
> >
> >
> > Review request for geode.
> >
> >
> > Bugs: GEODE-38
> > https://issues.apache.org/jira/browse/GEODE-38
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > Gfsh launcher script implies an init file can be used. This was possible
> in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF
> 7.0 onwards.
> > From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
> > "
> > #
> > Copy default .gfshrc to the home directory. Uncomment if needed.
> > #
> > #if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
> > cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
> > #fi
> > "
> > If this file is specified, it is currently ignored.
> >
> >
> > Diffs
> > -----
> >
> >
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java
> d9a396a
> >
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java
> d7dba5a
> >
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java
> PRE-CREATION
> >
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java
> PRE-CREATION
> >
> > Diff: https://reviews.apache.org/r/34814/diff/
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Roman Shaposhnik
> >
> >
>
>
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34814/#review85742
-----------------------------------------------------------
This looks good, with the exception that I think there needs to be an @After in the config file unit test that resets the system properties to what they were before the test ran.
- Bruce Schuchardt
On May 29, 2015, 3:39 p.m., Roman Shaposhnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34814/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 3:39 p.m.)
>
>
> Review request for geode.
>
>
> Bugs: GEODE-38
> https://issues.apache.org/jira/browse/GEODE-38
>
>
> Repository: geode
>
>
> Description
> -------
>
> Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
> From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
> "
> #
> Copy default .gfshrc to the home directory. Uncomment if needed.
> #
> #if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
> cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
> #fi
> "
> If this file is specified, it is currently ignored.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Roman Shaposhnik
>
>
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Roman Shaposhnik <rv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34814/
-----------------------------------------------------------
(Updated June 15, 2015, 8:21 p.m.)
Review request for geode.
Changes
-------
Publishing an updated patch
Bugs: GEODE-38
https://issues.apache.org/jira/browse/GEODE-38
Repository: geode
Description
-------
Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
>From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
"
#
Copy default .gfshrc to the home directory. Uncomment if needed.
#
#if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
#fi
"
If this file is specified, it is currently ignored.
Diffs (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileJUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/34814/diff/
Testing
-------
Thanks,
Roman Shaposhnik
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34814/#review85760
-----------------------------------------------------------
Please clean up the indentation.
Two spaces should be used for each level of indentation.
I saw some places that used tabs and those need to use spaces instead.
I saw some other places that indented with four spaces
- Darrel Schneider
On May 29, 2015, 8:39 a.m., Roman Shaposhnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34814/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 8:39 a.m.)
>
>
> Review request for geode.
>
>
> Bugs: GEODE-38
> https://issues.apache.org/jira/browse/GEODE-38
>
>
> Repository: geode
>
>
> Description
> -------
>
> Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
> From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
> "
> #
> Copy default .gfshrc to the home directory. Uncomment if needed.
> #
> #if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
> cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
> #fi
> "
> If this file is specified, it is currently ignored.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Roman Shaposhnik
>
>
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Neil Stevenson <ne...@em-chips.co.uk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34814/#review85914
-----------------------------------------------------------
Attached patch 0002 to https://issues.apache.org/jira/browse/GEODE-38
Three changes
o- Correcting code format for diffs for original files and full file for new files.
o- Renamed unit tests to match expected naming
o- Reset system properties to original values following tests
- Neil Stevenson
On May 29, 2015, 3:39 p.m., Roman Shaposhnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34814/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 3:39 p.m.)
>
>
> Review request for geode.
>
>
> Bugs: GEODE-38
> https://issues.apache.org/jira/browse/GEODE-38
>
>
> Repository: geode
>
>
> Description
> -------
>
> Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
> From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
> "
> #
> Copy default .gfshrc to the home directory. Uncomment if needed.
> #
> #if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
> cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
> #fi
> "
> If this file is specified, it is currently ignored.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Roman Shaposhnik
>
>
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Roman Shaposhnik <rv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34814/
-----------------------------------------------------------
(Updated May 29, 2015, 3:39 p.m.)
Review request for geode.
Bugs: GEODE-38
https://issue.apache.org/jira/browse/GEODE-38
Repository: geode
Description
-------
Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
>From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
"
#
Copy default .gfshrc to the home directory. Uncomment if needed.
#
#if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
#fi
"
If this file is specified, it is currently ignored.
Diffs
-----
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/34814/diff/
Testing
-------
Thanks,
Roman Shaposhnik
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Roman Shaposhnik <rv...@apache.org>.
Done!
On Fri, May 29, 2015 at 8:52 AM, Roman Shaposhnik <rv...@apache.org> wrote:
> Yeah that seems to have been a typo in a config. Let me look into this...
>
> Thanks,
> Roman.
>
> On Fri, May 29, 2015 at 8:41 AM, Anthony Baker <ab...@pivotal.io> wrote:
>> The link in this email is incorrect…should be “issues.apache.org” not “issue.apache.org”.
>>
>> Any idea how to fix it?
>>
>> Anthony
>>
>>
>>> On May 29, 2015, at 8:39 AM, Roman Shaposhnik <rv...@apache.org> wrote:
>>>
>>>
>>> -----------------------------------------------------------
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/34814/
>>> -----------------------------------------------------------
>>>
>>> (Updated May 29, 2015, 3:39 p.m.)
>>>
>>>
>>> Review request for geode.
>>>
>>>
>>> Bugs: GEODE-38
>>> https://issue.apache.org/jira/browse/GEODE-38
>>>
>>>
>>> Repository: geode
>>>
>>>
>>> Description
>>> -------
>>>
>>> Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
>>> From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
>>> "
>>> #
>>> Copy default .gfshrc to the home directory. Uncomment if needed.
>>> #
>>> #if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
>>> cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
>>> #fi
>>> "
>>> If this file is specified, it is currently ignored.
>>>
>>>
>>> Diffs
>>> -----
>>>
>>> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
>>> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
>>> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
>>> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
>>>
>>> Diff: https://reviews.apache.org/r/34814/diff/
>>>
>>>
>>> Testing
>>> -------
>>>
>>>
>>> Thanks,
>>>
>>> Roman Shaposhnik
>>>
>>
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Roman Shaposhnik <rv...@apache.org>.
Yeah that seems to have been a typo in a config. Let me look into this...
Thanks,
Roman.
On Fri, May 29, 2015 at 8:41 AM, Anthony Baker <ab...@pivotal.io> wrote:
> The link in this email is incorrect…should be “issues.apache.org” not “issue.apache.org”.
>
> Any idea how to fix it?
>
> Anthony
>
>
>> On May 29, 2015, at 8:39 AM, Roman Shaposhnik <rv...@apache.org> wrote:
>>
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/34814/
>> -----------------------------------------------------------
>>
>> (Updated May 29, 2015, 3:39 p.m.)
>>
>>
>> Review request for geode.
>>
>>
>> Bugs: GEODE-38
>> https://issue.apache.org/jira/browse/GEODE-38
>>
>>
>> Repository: geode
>>
>>
>> Description
>> -------
>>
>> Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
>> From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
>> "
>> #
>> Copy default .gfshrc to the home directory. Uncomment if needed.
>> #
>> #if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
>> cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
>> #fi
>> "
>> If this file is specified, it is currently ignored.
>>
>>
>> Diffs
>> -----
>>
>> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
>> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
>> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
>> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
>>
>> Diff: https://reviews.apache.org/r/34814/diff/
>>
>>
>> Testing
>> -------
>>
>>
>> Thanks,
>>
>> Roman Shaposhnik
>>
>
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Anthony Baker <ab...@pivotal.io>.
The link in this email is incorrect…should be “issues.apache.org” not “issue.apache.org”.
Any idea how to fix it?
Anthony
> On May 29, 2015, at 8:39 AM, Roman Shaposhnik <rv...@apache.org> wrote:
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34814/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 3:39 p.m.)
>
>
> Review request for geode.
>
>
> Bugs: GEODE-38
> https://issue.apache.org/jira/browse/GEODE-38
>
>
> Repository: geode
>
>
> Description
> -------
>
> Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
> From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
> "
> #
> Copy default .gfshrc to the home directory. Uncomment if needed.
> #
> #if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
> cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
> #fi
> "
> If this file is specified, it is currently ignored.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
> gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/34814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Roman Shaposhnik
>
Re: Review Request 34814: GEODE-38. Gfsh init script ignored
Posted by Roman Shaposhnik <rv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34814/
-----------------------------------------------------------
(Updated May 29, 2015, 3:39 p.m.)
Review request for geode.
Bugs: GEODE-38
https://issue.apache.org/jira/browse/GEODE-38
Repository: geode
Description
-------
Gfsh launcher script implies an init file can be used. This was possible in GF 6.6, but wasn't implemented in the transition to the new Gfsh in GF 7.0 onwards.
>From "gemfire-assembly/src/main/dist/bin/gfsh.bat " :
"
#
Copy default .gfshrc to the home directory. Uncomment if needed.
#
#if [ ! -f $HOME/.gemfire/.gfsh2rc ]; then
cp $GEMFIRE/defaultConfigs/.gemfire/.gfsh2rc $HOME
#fi
"
If this file is specified, it is currently ignored.
Diffs
-----
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/Gfsh.java d9a396a
gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfig.java d7dba5a
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshConfigInitFileTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/management/internal/cli/shell/GfshInitFileTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/34814/diff/
Testing
-------
Thanks,
Roman Shaposhnik