Daniel Erez has posted comments on this change. Change subject: core: introduce RemoveDiskSnapshotsCommand ......................................................................
Patch Set 10: (7 comments) http://gerrit.ovirt.org/#/c/26327/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java: Line 31: import org.ovirt.engine.core.compat.Guid; Line 32: import org.ovirt.engine.core.dao.DiskDao; Line 33: import org.ovirt.engine.core.dao.DiskImageDAO; Line 34: import org.ovirt.engine.core.dao.VmDeviceDAO; Line 35: > please remember to add the LockIdNameAttribute annotation Done Line 36: @NonTransactiveCommandAttribute Line 37: public class RemoveDiskSnapshotsCommand<T extends RemoveDiskSnapshotsParameters> extends BaseImagesCommand<T> Line 38: implements TaskHandlerCommand<RemoveDiskSnapshotsParameters> { Line 39: Line 103: protected boolean canDoAction() { Line 104: if (getVm() == null) { Line 105: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND); Line 106: } Line 107: > IIRC in RemoveDiskCommand and other operations like move we lock only the a 'validateAllDiskImages' method is already verifying the entire chain of disks Line 108: DiskSnapshotsValidator diskSnapshotsValidator = createDiskSnapshotsValidator(getImages()); Line 109: if (!validate(diskSnapshotsValidator.diskSnapshotsNotExist(getParameters().getImageIds())) || Line 110: !validate(diskSnapshotsValidator.diskImagesBelongToSameImageGroup()) || Line 111: !validate(diskSnapshotsValidator.imagesAreSnapshots())) { Line 126: return false; Line 127: } Line 128: Line 129: VmValidator vmValidator = createVmValidator(getVm()); Line 130: if (isDiskPlugged() && !validate(vmValidator.vmDown())) { > perhaps we can improve the message in that scneario (instead of getting "vm good idea, I'll do it in a following patch Line 131: return false; Line 132: } Line 133: Line 134: if (!validate(new StoragePoolValidator(getStoragePool()).isUp()) || http://gerrit.ovirt.org/#/c/26327/10/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java: Line 36: import org.ovirt.engine.core.dao.VmTemplateDAO; Line 37: import org.ovirt.engine.core.utils.MockConfigRule; Line 38: Line 39: /** A test case for the {@link RemoveDiskSnapshotsCommand} class. */ Line 40: @RunWith(MockitoJUnitRunner.class) > what about more test cases? :) most scenarios are already tested in the validators... Line 41: public class RemoveDiskSnapshotsCommandTest { Line 42: Line 43: private RemoveDiskSnapshotsCommand<RemoveDiskSnapshotsParameters> cmd; Line 44: Line 110: mockVm(); Line 111: Line 112: vmValidator = spy(new VmValidator(cmd.getVm())); Line 113: doReturn(vmValidator).when(cmd).createVmValidator(any(VM.class)); Line 114: doReturn(ValidationResult.VALID).when(vmValidator).vmNotHavingDeviceSnapshotsAttachedToOtherVms(false); > not relevant anymore (removed from the command) Done Line 115: Line 116: diskImagesValidator = spy(new DiskImagesValidator(mockImages())); Line 117: doReturn(diskImagesValidator).when(cmd).createDiskImageValidator(any(List.class)); Line 118: doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotExist(); Line 145: Line 146: private List<DiskImage> mockImages() { Line 147: DiskImage image1 = new DiskImage(); Line 148: image1.setImageId(IMAGE_ID_1); Line 149: image1.setPlugged(true); > unneeded Done Line 150: image1.setStorageIds(new ArrayList<>(Arrays.asList(STORAGE_DOMAIN_ID))); Line 151: Line 152: DiskImage image2 = new DiskImage(); Line 153: image2.setImageId(IMAGE_ID_2); Line 150: image1.setStorageIds(new ArrayList<>(Arrays.asList(STORAGE_DOMAIN_ID))); Line 151: Line 152: DiskImage image2 = new DiskImage(); Line 153: image2.setImageId(IMAGE_ID_2); Line 154: image2.setPlugged(true); > unneeded Done Line 155: image2.setStorageIds(new ArrayList<>(Arrays.asList(STORAGE_DOMAIN_ID))); Line 156: Line 157: return new ArrayList<>(Arrays.asList(image1, image2)); Line 158: } -- To view, visit http://gerrit.ovirt.org/26327 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia714a4390d1d9b672005be30f58b7fa98b9a31cd Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
