> -----Original Message----- > From: John Burwell [mailto:jburw...@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...@citrix.com> wrote: > > > > > > >> -----Original Message----- > >> From: Chip Childers [mailto:chip.child...@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
