Omer Frenkel has posted comments on this change. Change subject: core:(2) Editing of template version for pools ......................................................................
Patch Set 7: (7 comments) https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachUserFromVmFromPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachUserFromVmFromPoolCommand.java: Line 45: if (perm != null) { Line 46: DbFacade.getInstance().getPermissionDao().remove(perm.getId()); Line 47: if (getParameters().getIsRestoreStateless()) { Line 48: VM vm = DbFacade.getInstance().getVmDao().get(getParameters().getVmId()); Line 49: if (vm != null) { not sure i understand what you did here (except adding the parameter, looks like you just changed the order of getting the permission and the vm from db) but why did you remove the check that the vm belong to the pool? isn't it needed? is it covered somewhere else? Line 50: RestoreVmFromBaseSnapshot(vm); Line 51: } Line 52: } Line 53: } https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllPoolVmsQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllPoolVmsQuery.java: Line 17: Line 18: @Override Line 19: protected void executeQueryCommand() { Line 20: List<VM> vmsList = getDbFacade() Line 21: .getVmDao().getAllForVmPool(getParameters().getId()); since this is defined as a user query (is it used by user portal?), you need to filter the results Line 22: for (VM vm : vmsList) { Line 23: VmHandler.updateVmGuestAgentVersion(vm); Line 24: } Line 25: getQueryReturnValue().setReturnValue(vmsList); https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetLatestTemplateInChainQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetLatestTemplateInChainQuery.java: Line 11: Line 12: @Override Line 13: protected void executeQueryCommand() { Line 14: VmTemplate vmt; Line 15: vmt = DbFacade.getInstance().getVmTemplateDao().getTemplateWithLatestVersionInChain(getParameters().getId()); user query issue Line 16: if (vmt != null) { Line 17: VmTemplateHandler.updateDisksFromDb(vmt); Line 18: VmHandler.updateVmInitFromDB(vmt, true); Line 19: } https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmTemplatesByBaseTemplateIdQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmTemplatesByBaseTemplateIdQuery.java: Line 15: Line 16: @Override Line 17: protected void executeQueryCommand() { Line 18: List<VmTemplate> templateList = Line 19: DbFacade.getInstance().getVmTemplateDao().getTemplateVersionsForBaseTemplate(getParameters().getId()); user query issue Line 20: if (templateList != null) { Line 21: VmTemplate baseTemplate = DbFacade.getInstance().getVmTemplateDao().get(getParameters().getId(), getUserID(), getParameters().isFiltered()); Line 22: if (baseTemplate != null) { Line 23: templateList.add(baseTemplate); https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 186: setSucceeded(true); Line 187: } Line 188: Line 189: private void updateVmTemplateVersion() { Line 190: if (getVm().getStatus() == VMStatus.Down) { // if vm is down -> updateVmVersionCommand will handle the rest. if it's not, a NEXT_RUN snapshot will be created. maybe move this comment to java doc for the method, and add little more info Line 191: VdcReturnValueBase result = Line 192: runInternalActionWithTasksContext( Line 193: VdcActionType.UpdateVmVersion, Line 194: new UpdateVmVersionParameters(getVmId(), https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmVersionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmVersionCommand.java: Line 60: super(parameters, cmdContext); Line 61: parameters.setEntityInfo(new EntityInfo(VdcObjectType.VM, parameters.getVmId())); Line 62: Line 63: if (getVm() != null) { Line 64: if(parameters.getNewTemplateVersion() != null) { missing space "if (" Line 65: setVmTemplate(getVmTemplateDAO().get(parameters.getNewTemplateVersion())); Line 66: } else { Line 67: setVmTemplate(getVmTemplateDAO().getTemplateWithLatestVersionInChain(getVm().getVmtGuid())); Line 68: } https://gerrit.ovirt.org/#/c/36508/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/UpdateVmVersionParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/UpdateVmVersionParameters.java: Line 7: private Guid vmPoolId; Line 8: /** The ID of disk operator of one of the disks the VM had before the update */ Line 9: private Guid previousDiskOperatorAuthzPrincipalDbId; Line 10: private Guid newTemplateVersion; Line 11: private Boolean LatestStatus; why not boolean? also maybe rename to useLatestVersion? what status means? Line 12: Line 13: public UpdateVmVersionParameters() { Line 14: } Line 15: -- To view, visit https://gerrit.ovirt.org/36508 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2249cfea12ffbab008c162d52928df782f5f9d85 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
