Arik Hadas has posted comments on this change. Change subject: core: Use import command to register a VM ......................................................................
Patch Set 18: (5 comments) http://gerrit.ovirt.org/#/c/27247/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 243: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_IMPORT_CLONE_NOT_COLLAPSED); Line 244: } Line 245: Line 246: if (isImagesAlreadyOnTarget() && getParameters().getCopyCollapse()) { Line 247: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_IMPORT_CLONE_NOT_COLLAPSED); copy-paste.... :) should be ACTION_TYPE_FAILED_IMPORT_UNREGISTERED_NOT_COLLAPSED right? Line 248: } Line 249: Line 250: setSourceDomainId(getParameters().getSourceDomainId()); Line 251: StorageDomainValidator validator = new StorageDomainValidator(getSourceDomain()); Line 298: } Line 299: } Line 300: } else { Line 301: // If no copy collapse sent, validate each image configuration (snapshot or active image). Line 302: if (!validateImageConfig(canDoActionMessages, domainsMap, image)) { do we need to execute this validation? Line 303: return false; Line 304: } Line 305: } Line 306: Line 607: } Line 608: Line 609: private boolean verifyDisksIfNeeded() { Line 610: boolean verified = true; Line 611: if (!getParameters().isImportAsNewEntity() && !isImagesAlreadyOnTarget()) { I think this method is redundant: wouldn't it be simpler to return true in checkIfDisksExist when isImagesAlreadyOnTarget()=true ? Line 612: verified = checkIfDisksExist(imageList); Line 613: } Line 614: return verified; Line 615: } Line 656: protected void executeCommand() { Line 657: executeImportVm(!isImagesAlreadyOnTarget()); Line 658: } Line 659: Line 660: protected void executeImportVm(boolean useCopyImages) { why did you extract this method? unless I'm missing something, it is called in a similar way in this command and the new one that extends it.. I prefer not to introduce new method but call !isImagesAlreadyOnTarget() in #701 Line 661: try { Line 662: addVmToDb(); Line 663: processImages(useCopyImages); Line 664: // if there aren't tasks - we can just perform the end http://gerrit.ovirt.org/#/c/27247/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java: Line 55: if (!getStorageDomain().getStorageDomainType().isDataDomain()) { Line 56: addCanDoActionMessage("$domainId " + getParameters().getStorageDomainId()); Line 57: addCanDoActionMessage("$domainType " + getStorageDomain().getStorageDomainType()); Line 58: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_UNSUPPORTED); Line 59: return false; can you replace #56-#59 with a call to failCanDoAction ? Line 60: } Line 61: } Line 62: return super.canDoAction(); Line 63: } -- To view, visit http://gerrit.ovirt.org/27247 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5ee64651eb614feb6ac9d7fde88a4ee6348aff06 Gerrit-PatchSet: 18 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@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: 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
