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? 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. 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
