How about, for all the interfaces, DB schema changes, related to storage subsystem, need to send out a merge request and push the patches into review board? It's not only for feature development, but also for bug fixes. I am not sure how many people want to review the changes related to storage subsystem, though. If only I am interested in it, then don't need to do that:)
> -----Original Message----- > From: John Burwell [mailto:jburw...@basho.com] > Sent: Friday, June 21, 2013 1:00 PM > 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, > > 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...@citrix.com> wrote: > > > > > > >> -----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 > >
