All, While I am stating the obvious, but we must remember that commit/PR flow is the lifeblood our community. Over the past the month, there have been zero contributions to master [1] against 180 open PRs. As Simon points out, the longer the PR sits, the farther it diverges — increasing the risk that the submitter will give up and we will lose valuable work. It is also very difficult to cultivate new contributors if their PRs are not addressed in a timely manner.
While I think the work being done on CI is extremely valuable and vital in the long term, it seems appropriate that we decouple it from PR review and merge. Looking through the wiki, we have great instructions on the mechanics of properly merging a PR [2], but it provides little guidance about the steps to be performed to complete a PR review. Fleshing out these guidelines will not only provide us with a manual process we can use until a CI infrastructure is available, it will also allow us to verify the process implement by CI. Currently, we require 2 LGTM votes — at least one for code review and at least one for testing. To my mind, a code review LGTM should include successfully passing the following items: 1. Verifying the branch passes all static analysis checks (currently checked by Travis) 2. Verifying the branch passes all unit tests (currently checked by Travis) 3. Manual inspection of the code for issues such as architectural and class design issues, Java best practices, concurrency problems, and unit test completeness 4. Any new Maven dependencies in redist modules have a license compatible with the ASL, have no open CVEs, and do not unnecessarily duplicate functionality of an existing dependency For a test LGTM, the branch should be checked out and verified to run. In addition to running tests, the PR should be review to ensure that it updates existing Marvin tests and/or adds new Marvin test cases to cover the changes. It should then pass any Marvin tests added or modified for the change, as well as, all unit tests. I think it would be beneficial to identify the set acceptance tests that should pass dependent on the nature of the PR. For example, if a PR affects KVM, there should be a basic set of tests that should pass in order to the PR to be merged to master. Ideally, we would run all tests. Unfortunately, the entire suite takes ~7 hours to run making it impractical to run for every test. Hopefully, with the CI infrastructure and test improvements, it will eventually become feasible. The following are the acceptance test impact areas that come to mind: 1. Core (always run no matter what the change) 2. KVM 3. VMWare 4. XenServer 5. Basic Networking 6. Advanced Networking/VR 7. NFS 8. iSCSI 9. VPC These categories may not be the best or there may be more. Initially, I would propose simply listed them in the wiki or providing a simple script. We can then go back and take the time to update the category(ies) in Marvin. The goal is to ensure we consistently test PRs and we set clear/realistic expectations for contributors. It also far more likely that contributors will run these tests in advance if they it is clearly explained which tests should pass. I am willing to take up the effort to update the wiki with the process, as well as, working through the acceptance test lists. I need feedback on an initial list of categories — I think erring towards too few rather too many would be a wise approach. With that information, I will expand the existing topic to include the testing information, as well as, the code review expectations. While I believe it will be very helpful to flesh the PR review process, I don’t think any of these steps should hold back PR review. As Remi pointed out (and did as release manager), code reviewing and running a base set of Marvin tests would be an excellent starting point to start getting the flow moving immediately. We can apply the improvements from CI and expanded review guidelines as they become available. Thanks, -John [1]: https://github.com/apache/cloudstack/graphs/contributors?from=2016-02-15&to=2016-03-18&type=c [2]: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61311655 > [ShapeBlue]<http://www.shapeblue.com> John Burwell ShapeBlue d: +44 (20) 3603 0542 | s: +1 (571) 403-2411 <tel:+44%20(20)%203603%200542%20|%20s:%20+1%20(571)%20403-2411> e: john.burw...@shapeblue.com | t: <mailto:john.burw...@shapeblue.com%20|%20t:> | w: www.shapeblue.com<http://www.shapeblue.com> a: 53 Chandos Place, Covent Garden London WC2N 4HS UK [cid:image126380.png@e29adf1a.4a908ea2] Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The Republic of South Africa and is traded under license from Shape Blue Ltd. ShapeBlue is a registered trademark. This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error. On Mar 17, 2016, at 5:16 PM, Will Stevens <williamstev...@gmail.com> wrote: > > I am trying to work through this. Hopefully I can get my CI setup for next > week and start getting through the backlog. Any assistance testing will be > appreciated. > > Also, any fixes to marvin tests will take priority of any other PRs. > On Mar 17, 2016 5:06 PM, "ilya" <ilya.mailing.li...@gmail.com> wrote: > >> Hi Folks, >> >> What can we do about PR backlog in GitHub? As we all know, it will be >> very difficult to merge the changes - as things will get out of sync. >> >> Feedback is welcome, >> >> Thanks, >> ilya >> Find out more about ShapeBlue and our range of CloudStack related services: IaaS Cloud Design & Build<http://shapeblue.com/iaas-cloud-design-and-build//> | CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/> | CloudStack Software Engineering<http://shapeblue.com/cloudstack-software-engineering/> CloudStack Infrastructure Support<http://shapeblue.com/cloudstack-infrastructure-support/> | CloudStack Bootcamp Training Courses<http://shapeblue.com/cloudstack-training/>
