cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Burwell <jburw...@basho.com>
Subject Re: [DISCUSS] Do we need code review process for code changes related to storage subsystem?
Date Fri, 21 Jun 2013 19:59:58 GMT
Edison,

The person pushing the merge request should highlight that it includes interface changes (regardless
of whether or not it is a storage patch).  I also think that we should be pushing all patches
that rise to merge requests into Review Board to allow potential reviewers a place to cleanly
communicate and discuss issues.  

Thanks,
-John

On Jun 21, 2013, at 3:53 PM, Edison Su <Edison.su@citrix.com> wrote:

> 
> 
>> -----Original Message-----
>> From: John Burwell [mailto:jburwell@basho.com]
>> Sent: Friday, June 21, 2013 11:43 AM
>> To: dev@cloudstack.apache.org
>> Cc: 'Chip Childers'
>> Subject: Re: [DISCUSS] Do we need code review process for code changes
>> related to storage subsystem?
>> 
>> Edison,
>> 
>> My understanding of our process is that the merges of significant changes
>> should be proposed to the mailing list with the "[MERGE]" tag and wait up to
>> 72 hours for feedback.  I consider interface changes to meet that criteria
>> given the potential to break other folks work.  It sounds like we had a change
>> that inadvertently slipped through without notice to list.  Going forward, I
> 
> The problem of current merge request, is that, you don't know what kind of change the
merge request did, unless you dig into the code.
> Let's say, there is a merge request, the code touches both vm and storage code, then
how do I know, by just taking look at the request, that, the storage part of code is been
changed.
> As there are lot of merge requests, a single person can't review all the merge requests,
then likely, the change is slipped without the notice to other people who wants to review
storage related code, even if the merge request is send out to the list.
> 
> Maybe, we should ask for each merge request, need to add a list of files been changed:
like the output of "git diff --stat"?
> 
>> propose that we follow this process for significant patches and, additionally,
>> push them to Review Board.  As a matter of collaboration, it might also be a
>> good idea to open a "[DISCUSS]" thread during the design/planning stages,
>> but I don't think we need to require it.
>> 
>> Do you think this approach will properly balance to our needs to
>> coordinate/review work with maintaining a smooth flow?
>> 
>> Thanks,
>> -John
>> 
>> 
>> On Jun 21, 2013, at 2:15 PM, Edison Su <Edison.su@citrix.com> wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Chip Childers [mailto:chip.childers@sungard.com]
>>>> Sent: Thursday, June 20, 2013 5:42 PM
>>>> To: dev@cloudstack.apache.org
>>>> Cc: Edison Su
>>>> Subject: Re: [DISCUSS] Do we need code review process for code
>>>> changes related to storage subsystem?
>>>> 
>>>> On Thu, Jun 20, 2013 at 05:59:01PM +0000, Edison Su wrote:
>>>>> For interface/API changes, we'd better have a code review, as more
>>>> storage vendors and more developers outside Citrix are contributing
>>>> code to CloudStack storage subsystem. The code change should have
>>>> less surprise for everybody who cares about storage subsystem.
>>>> 
>>>> I'm not following what you are saying Edison.  What are we not doing
>>>> in this regard right now?  I'm also not getting the "Citrix" point of
>> reference here.
>>> 
>>> We don't have a code review process for each commit currently, the result
>> of that, as the code evolving, people add more and more code, features, bug
>> fixes etc, etc. Then maybe one month later, when you take a look at the
>> code, which may be quite different than what you known about. So I want to
>> add a code review process here, maybe start from storage subsystem at first.
>>> The reason I add "Citrix" here, let's take a look at what happened in the last
>> month:
>>> Mike, from SolidFire,  is asking why there is a hypervisor field in the storage
>> pool, simply, the hypervisor field breaks his code.
>>> And I don't understand why there is a column, called  dynamicallyScalable,
>> in vm_template table.
>>> I think you will understand my problem here...
>>> 
>>> 
>>> 
>>>> 
>>>> -chip
> 


Mime
View raw message