Allon Mureinik has posted comments on this change.
Change subject: core: restapi: Introducing ImportVmFromConfigurationCommand
......................................................................
Patch Set 8: (8 inline comments)
Looks good!
some minor comments inline.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmFromConfigurationQuery.java
Line 15: try {
Line 16:
getQueryReturnValue().setReturnValue(ovfHelper.readVmFromOvf(getParameters().getVmConfiguration()));
Line 17: getQueryReturnValue().setSucceeded(true);
Line 18: } catch (Exception e) {
Line 19: log.debug("failed to parse a given ovf configuration",
e);
Consider added a log.debug of the failing given OVF
Line 20: getQueryReturnValue().setExceptionString("failed to
parse a given ovf configuration" + e.getMessage());
Line 21: }
Line 22: }
Line 23: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 115:
Line 116: @Override
Line 117: protected void init(T parameters) {
Line 118: super.init(parameters);
Line 119: setVmId(getParameters().getContainerId());
please double check if your really need to use getParameters(), and can't just
use "parameters"
Line 120: setVm(getParameters().getVm());
Line 121: setVdsGroupId(getParameters().getVdsGroupId());
Line 122: if (getParameters().getVm() != null && getVm().getDiskMap()
!= null) {
Line 123: imageList = new ArrayList<DiskImage>();
Line 139: }
Line 140:
Line 141: @Override
Line 142: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 143: if (getParameters().getVm() != null &&
!StringUtils.isBlank(getParameters().getVm().getName())) {
the parameters object has a @Valid on it's VM - please double check that this
does not ensure that it's not null
Line 144: return
Collections.singletonMap(getParameters().getVm().getName(),
Line 145:
LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM_NAME,
VdcBllMessages.ACTION_TYPE_FAILED_NAME_ALREADY_USED));
Line 146: }
Line 147: return null;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java
Line 48:
Line 49: setVdsGroupId(getParameters().getVdsGroupId());
Line 50:
getParameters().setStoragePoolId(getVdsGroup().getStoragePoolId());
Line 51: super.init(parameters);
Line 52: }
you removed the canDoAction() that checks if
getParameters().getVm() == null.
Shouldn't it be added to ImportCommandBase?
Line 53:
Line 54: @Override
Line 55: public void executeCommand() {
Line 56: super.executeCommand();
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmsResourceTest.java
Line 51: import org.ovirt.engine.core.common.queries.VdcQueryType;
Line 52: import org.ovirt.engine.core.common.utils.VmDeviceType;
Line 53: import org.ovirt.engine.core.compat.Guid;
Line 54:
Line 55: import static org.easymock.EasyMock.expect;
please fix your imports organization in your IDE
Line 56:
Line 57: public class BackendVmsResourceTest
Line 58: extends AbstractBackendCollectionResourceTest<VM,
org.ovirt.engine.core.common.businessentities.VM, BackendVmsResource> {
Line 59:
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/VmMapperTest.java
Line 24: import org.ovirt.engine.core.common.config.ConfigValues;
Line 25: import org.ovirt.engine.core.compat.Guid;
Line 26: import org.ovirt.engine.core.compat.Version;
Line 27:
Line 28: import junit.framework.Assert;
import ordering...
Line 29:
Line 30: public class VmMapperTest extends
Line 31: AbstractInvertibleMappingTest<VM, VmStatic,
org.ovirt.engine.core.common.businessentities.VM> {
Line 32:
....................................................
Commit Message
Line 3: AuthorDate: 2013-07-04 16:50:55 +0300
Line 4: Commit: Liron Aravot <[email protected]>
Line 5: CommitDate: 2013-07-25 14:58:56 +0300
Line 6:
Line 7: core: restapi: Introducing ImportVmFromConfigurationCommand
s/core: restapi:/core,restapi:/
Line 8:
Line 9: ImportVmFromConfigurationCommand is used for adding a vm to the engine
Line 10: with a configuration specified in an ovf file.
Line 11: The command adds the vm and related devices as specified in the given
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml
Line 202: <include name="common/businessentities/VdcRole.java" />
Line 203:
Line 204: <include
name="common/businessentities/FenceActionType.java" />
Line 205: <include
name="common/businessentities/CopyVolumeType.java" />
Line 206: <include
name="common/businessentities/ConfigurationType.java" />
please align properly
Line 207: <include
name="common/businessentities/ImageOperation.java" />
Line 208: <include
name="common/businessentities/ImageDbOperationScope.java" />
Line 209: <include name="common/businessentities/VdcOption.java"
/>
Line 210: <include name="common/errors/VdcFault.java" />
--
To view, visit http://gerrit.ovirt.org/15894
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9390bcc11e5f3cb8c8c89ce791aabe63fe0ca341
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <[email protected]>
Gerrit-Reviewer: A Burden <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
