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 [email protected] or file a JIRA ticket
with INFRA.
---
