[GitHub] incubator-geode issue #171: feature/geode 33

[upthewaterspout <gi...@git.apache.org>]

Github user upthewaterspout commented on the issue:

    https://github.com/apache/incubator-geode/pull/171
  
    Hi William,
    
    Thanks for doing all of this work! This looks like a good start for examples. I have a few comments below.
    
    1. Did you look into using the gradle application plugin, rather than writing a custom run task?
    
    2. We'll need to verify that the gradle wrapper jar doesn't become part of the source distribution. I guess we need to do the same thing with this gradle wrapper jar that we did at the top level of the project.
    
    3. I wonder if this ShellUtil class should be put into some common utility project for these example tests. 
    
    4. I  see a problem with the ShellUtil code; it's not consuming the output of the process. Especially on windows this can cause bizarre hangs because the process will block trying to write to stdout. Also it makes it really hard to debug failures if you all you do is assert that the status is 0 but don't provide the output of the process. The code should probably include the process output in the error message if the process returns a non-zero exit.
    
    5. 'geode-examples/*/scripts/**' should not be excluded from rat.
    
    6. How should we integrate this with the rest of the geode build? I think these examples should probably get shipped as part of the source and binary distributions, and the the examples should be built and tested as part of precheckin.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---