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 >
