You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by ilya <il...@gmail.com> on 2016/03/17 22:06:34 UTC
[IMPORTANT] Huge Github PR Backlog
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
Re: [IMPORTANT] Huge Github PR Backlog
Posted by Jeff Hair <je...@greenqloud.com>.
Sometimes they might seem old, but when interaction just sort of... stops,
it can be hard to keep track of what is going on with the PRs. Currently we
have 6 or 7 open PRs that no one has commented on or looked at in months.
Two were recently updated with integration test results at least. One is
missing a unit test that I still need to write, but the others are (were)
ready to go. Every few weeks I go and rebase them and then they sit there
for a few more weeks. Rinse, repeat it seems like...
What scope is needed for the testing? Is the plan to use some CI then
manually test, in addition to the unit tests? Or are in some cases unit
tests good enough?
On Thu, Mar 17, 2016 at 9:18 PM, Remi Bergsma <RB...@schubergphilis.com>
wrote:
> PRs needs to be tested obviously, that is the current impediment. The
> moment you test a PR, you get current master and merge the PR and test that
> result. Once it’s OK, merge it to master and take next one, etc. I wouldn’t
> start rebasing, as you’ll have a lot of work and will need to keep doing
> that. Git is pretty good in merging stuff so it will work quite often. Or
> else result in some PRs not being able to merge due to conflicts. Those
> need to be resolved.
>
> Another thing: I would say 50% (if not more) is never going to be merged
> because they’re too old, abandoned, broken, etc.
>
> Regards,
> Remi
>
>
>
>
>
> On 17/03/16 22:10, "Simon Weller" <sw...@ena.com> wrote:
>
> >If we ask for each PR to be rebased with master, we need to commit as a
> team to actually working through them rapidly, as obviously rebasing is
> significant work in a lot of cases.
> >
> >Just my 2 cents.
> >
> >- Si
> >
> >
> >
> >
> >________________________________________
> >From: ilya <il...@gmail.com>
> >Sent: Thursday, March 17, 2016 4:06 PM
> >To: dev@cloudstack.apache.org
> >Subject: [IMPORTANT] Huge Github PR Backlog
> >
> >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
>
Re: [IMPORTANT] Huge Github PR Backlog
Posted by Remi Bergsma <RB...@schubergphilis.com>.
PRs needs to be tested obviously, that is the current impediment. The moment you test a PR, you get current master and merge the PR and test that result. Once it’s OK, merge it to master and take next one, etc. I wouldn’t start rebasing, as you’ll have a lot of work and will need to keep doing that. Git is pretty good in merging stuff so it will work quite often. Or else result in some PRs not being able to merge due to conflicts. Those need to be resolved.
Another thing: I would say 50% (if not more) is never going to be merged because they’re too old, abandoned, broken, etc.
Regards,
Remi
On 17/03/16 22:10, "Simon Weller" <sw...@ena.com> wrote:
>If we ask for each PR to be rebased with master, we need to commit as a team to actually working through them rapidly, as obviously rebasing is significant work in a lot of cases.
>
>Just my 2 cents.
>
>- Si
>
>
>
>
>________________________________________
>From: ilya <il...@gmail.com>
>Sent: Thursday, March 17, 2016 4:06 PM
>To: dev@cloudstack.apache.org
>Subject: [IMPORTANT] Huge Github PR Backlog
>
>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
Re: [IMPORTANT] Huge Github PR Backlog
Posted by Simon Weller <sw...@ena.com>.
If we ask for each PR to be rebased with master, we need to commit as a team to actually working through them rapidly, as obviously rebasing is significant work in a lot of cases.
Just my 2 cents.
- Si
________________________________________
From: ilya <il...@gmail.com>
Sent: Thursday, March 17, 2016 4:06 PM
To: dev@cloudstack.apache.org
Subject: [IMPORTANT] Huge Github PR Backlog
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
Re: [IMPORTANT] Huge Github PR Backlog
Posted by John Burwell <jo...@shapeblue.com>.
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.burwell@shapeblue.com | t: <mailto:john.burwell@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 <wi...@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" <il...@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/>
Re: [IMPORTANT] Huge Github PR Backlog
Posted by Will Stevens <wi...@gmail.com>.
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" <il...@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
>