Maor Lipchuk 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.... :) indeed, fixed 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? probably not, fixed 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 we should use this in the future, when we will want to create a new entity from OVF with different GUIDs 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 good point, I wanted to use this for indicating useCopyImages in this code scope, but I guess it will make more sense if it will be the same as it is used for template 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 ? done 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
