[GitHub] bigtop pull request: BIGTOP-2435 Add Juju charm, bundle, and testplan source...

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

Github user c0s commented on the pull request:

    https://github.com/apache/bigtop/pull/108
  
    I have reviewed the [dev@ thread discussion](https://lists.apache.org/thread.html/Z2uoq25segpbsm6) on this issue and while I agree with Olaf, that commit is a bit on the large side, it seems to be doing all the correct things. I believe the directory layout also makes sense. And the patch is passes the RAT too.
    
    It might makes sense to add weather-report part separately, but after all it is just one tiny file and a README in there, so it doesn't make that much difference. 
    
    The only real comment I can make is about 2 vs 4 spaces of the indentation as we are using the former. However, the majority of the new code is Python, so I am not really sure if 2-spaces indents look good (or even are acceptable) in that language.
    
    Hence, I am all +1 on getting this committed, unless there are hard objections from the rest of the community?


---
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.
---