On Jan 29, 2013, at 9:20 PM, Mice Xia <mice_...@tcloudcomputing.com> wrote:
> Hi, Chip > > 1) You state that it's based off of 4.0. Do you need to rebase against > master? > [mice] already rebased against master > > 2) Are you using reviewboard as a method for easy review of the changes, but > will be committing to master via a merge in the repo? Or will you pull the > patch into master from reviewboard? I think that once you get consensus on > the changes, you have commit rights to do either. > [mice] will commit to master via a merge in the repo > > 3) Are there any unit tests that test the new classes and methods? > [mice] Yes, VMSnapshotManagerTest.java for testing creation case in > VMSnapshotManager. But I'm not sure if this is sufficient, do we have some > guideline about unit test coverage? We don't, other than a general consensus to improve coverage whenever possible. ;) > > 4) Are there any doc updates required? > [mice] test scenarios need to be elaborated in the spec, currently I only > listed some fast acceptance cases. > I was thinking about the admin guide mostly. It's certainly not required before bringing in the feature, but it should probably get documented before the release. Sounds like your ready to merge! +1 from me > Regards > -Mice > > -----Original Message----- > From: Chip Childers [mailto:chip.child...@sungard.com] > Sent: Tuesday, January 29, 2013 10:34 PM > To: cloudstack-dev@incubator.apache.org > Subject: Re: [MERGE] Support VM Snapshot > > On Mon, Jan 28, 2013 at 9:35 PM, Mice Xia <mice_...@tcloudcomputing.com> > wrote: >> I would like to request merge of feature VM snapshot to master >> >> Review board: >> https://reviews.apache.org/r/9118/ >> >> Doc/spec >> https://cwiki.apache.org/confluence/display/CLOUDSTACK/VM+Snapshots >> >> Jira ticket >> https://issues.apache.org/jira/browse/CLOUDSTACK-684 >> >> Branch >> vm-snapshot (which is based on 4.0.0) >> >> Potential impact and notes: >> * 4 new states are added to VM state Machine >> * the case 'migrating volume to other primary storage with associated >> VM snapshots' is not yet handled >> * KVM support is not merged due to expected libvirt library has not released. >> * UI change is included. >> >> >> Regards >> Mice > > I'd hope for one of the more knowledgeable engineers to review the > implementation details, but the patch looks reasonable. > > Some questions though: > > 1) You state that it's based off of 4.0. Do you need to rebase against > master? > 2) Are you using reviewboard as a method for easy review of the changes, but > will be committing to master via a merge in the repo? Or will you pull the > patch into master from reviewboard? I think that once you get consensus on > the changes, you have commit rights to do either. > 3) Are there any unit tests that test the new classes and methods? > 4) Are there any doc updates required? > > -chip
