cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wilderrodrigues <...@git.apache.org>
Subject [GitHub] cloudstack pull request: Embedded Tomcat & Jetty
Date Tue, 09 Jun 2015 06:09:31 GMT
Github user wilderrodrigues commented on the pull request:

    https://github.com/apache/cloudstack/pull/372#issuecomment-110239298
  
    Hi there @rsafonseca 
    
    Cool to see you contribution, thanks for that, but I think the PRs should be broken down.
 You listed 11 changes and said that you might have forgotten another few. That's a bit dangerous
and also make testing the PR way to expansive for the reviewers.
    
    We should have 1 PR per feature/fix, or perhaps a couple of fixes, depending on their
size (fixing findbugs is a good example).
    
    Another good practice is to add the environment where the things were tested, and also
the steps you took to test it, in the PR. Adding those afterwards, as comments, it's also
not very good to follow.
    
    Reading your list I would say that you have at least 4 PRs.
    
    1. Tomcat/Jetty changes
    2. Packaging
    3. Configs and Distros
    4. JDK version
    
    In addition, the a PR should be independent of any other. Thus, we could for example test
4 and merge it without waiting for 2 or 3.
    
    If PR follows that structure I think we can get them merge way faster than now.
    
    I hope you appreciate the feedback. We are working to achieve the same goal.
    
    Cheers,
    Wilder


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

Mime
View raw message