> -----Original Message----- > From: Chip Childers [mailto:chip.child...@sungard.com] > Sent: Friday, June 21, 2013 1:22 PM > To: <dev@cloudstack.apache.org> > Subject: Re: [DISCUSS] Do we need code review process for code changes > related to storage subsystem? > > On Jun 21, 2013, at 4:18 PM, Edison Su <edison...@citrix.com> wrote: > > > 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:) > > I don't understand why storage is different from the rest of the code.
Because, there is no other body call for reviewing the change before. If we can make it as a standard process for all the changes related to interfaces, db changes, on cloudstack code, and there are people like to review the changes, then let's do it. > > > > >> -----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 > > > >
