Maor Lipchuk has posted comments on this change. Change subject: core: Allow creation of new disks as copy of any existing disk ......................................................................
Patch Set 3: (8 comments) Few general comments: 1. What about Quota, where do we check it for copy for VM? 2. The message should be deleted also from 2 other appErrors.properties files https://gerrit.ovirt.org/#/c/38334/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java: Line 134 Line 135 Line 136 Line 137 Line 138 This is not relevant anymore, please remove the comment Line 181: protected boolean checkIfNeedToBeOverride() { Line 182: if (isTemplate() && Line 183: getParameters().getOperation() == ImageOperation.Copy && Line 184: !getParameters().getForceOverride() && Line 185: getImage().getStorageIds().contains(getStorageDomain().getId())) { Please consider to extract this to a separate method Line 186: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_ALREADY_EXISTS); Line 187: } Line 188: return true; Line 189: } Line 337: * The following method will override a parameters which are not relevant for the MoveOrCopyDiskCommand to the Line 338: * correct values for these scenario in order to be used at parent class Line 339: */ Line 340: private void overrideParameters() { Line 341: boolean isTemplate = isTemplate(); style: It is pretty confusing calling the variable with the same name of the method, I would simply call the method with no additional variable or change the variable name Line 342: Line 343: if (getParameters().getOperation() == ImageOperation.Copy) { Line 344: getParameters().setUseCopyCollapse(true); Line 345: getParameters().setAddImageDomainMapping(true); Line 346: Line 347: if (!isTemplate) { Line 348: getParameters().setAddImageDomainMapping(false); Line 349: Line 350: Guid newId = Guid.newGuid(); please rename this variable to newImageId Line 351: Guid newGroupId = Guid.newGuid(); Line 352: Line 353: DiskImage image = getImage(); Line 354: Line 347: if (!isTemplate) { Line 348: getParameters().setAddImageDomainMapping(false); Line 349: Line 350: Guid newId = Guid.newGuid(); Line 351: Guid newGroupId = Guid.newGuid(); Please call this newId, since we don't really use group id any more, only id. Line 352: Line 353: DiskImage image = getImage(); Line 354: Line 355: image.setId(newGroupId); Line 350: Guid newId = Guid.newGuid(); Line 351: Guid newGroupId = Guid.newGuid(); Line 352: Line 353: DiskImage image = getImage(); Line 354: redundant empty line Line 355: image.setId(newGroupId); Line 356: image.setImageId(newId); Line 357: Line 358: image.setDiskAlias(getDiskAlias()); Line 361: image.setQuotaId(getParameters().getQuotaId()); Line 362: image.setDiskProfileId(getParameters().getDiskProfileId()); Line 363: Line 364: getParameters().setDestinationImageId(newId); Line 365: getParameters().setImageGroupID(getParameters().getImageGroupID()); I don't understand this line, you are setting the parameter which you are getting the value from? or I'm missing something? Line 366: getParameters().setDestImageGroupId(newGroupId); Line 367: } Line 368: } else { Line 369: getParameters().setUseCopyCollapse(false); Line 362: image.setDiskProfileId(getParameters().getDiskProfileId()); Line 363: Line 364: getParameters().setDestinationImageId(newId); Line 365: getParameters().setImageGroupID(getParameters().getImageGroupID()); Line 366: getParameters().setDestImageGroupId(newGroupId); Please think about refactor this to a separate method Line 367: } Line 368: } else { Line 369: getParameters().setUseCopyCollapse(false); Line 370: } -- To view, visit https://gerrit.ovirt.org/38334 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07eece05b1880011f1a1b4300d07f0bd1ebf30bd Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
