cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Burwell <>
Subject Re: [IMPORTANT] Huge Github PR Backlog
Date Fri, 18 Mar 2016 12:54:27 GMT

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.




John Burwell

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: | t: <|%20t:>
    |      w:<>

a:      53 Chandos Place, Covent Garden London WC2N 4HS UK


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
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 <> 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" <> 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<> |
CSForge – rapid IaaS deployment framework<>
CloudStack Consulting<> | CloudStack Software
CloudStack Infrastructure Support<>
| CloudStack Bootcamp Training Courses<>

  • Unnamed multipart/related (inline, None, 0 bytes)
View raw message