Alissa Bonas has posted comments on this change. Change subject: core: use template version on add vm/pool ......................................................................
Patch Set 1: (6 comments) .................................................... Commit Message Line 5: CommitDate: 2014-01-05 18:17:23 +0200 Line 6: Line 7: core: use template version on add vm/pool Line 8: Line 9: When adding stateless vm, or vm-pool, When adding stateless vm --> When adding a stateless vm Line 10: if using latest version, we need to get latest version available and use Line 11: it. Line 12: http://www.ovirt.org/index.php?title=Features/Template_Versions Line 13: Line 6: Line 7: core: use template version on add vm/pool Line 8: Line 9: When adding stateless vm, or vm-pool, Line 10: if using latest version, we need to get latest version available and use if using latest version --> if using the latest version of a template Line 11: it. Line 12: http://www.ovirt.org/index.php?title=Features/Template_Versions Line 13: Line 14: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1037478 .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java Line 110: if (parameters.getVmStaticData() != null) { Line 111: Guid templateIdToUse = getParameters().getVmStaticData().getVmtGuid(); Line 112: Line 113: if (getParameters().getVmStaticData().getTemplateVersion() == null) { Line 114: VmTemplate latest = getVmTemplateDAO().getTemplateWithLatestVersionInChain(templateIdToUse); Can the latest template be null? Even if there are no different template versions (only one version exists), the base version be returned in this case, no? Line 115: Line 116: if (latest != null) { Line 117: // if not using original template, need to override storage mappings Line 118: // as it may have different set of disks Line 121: } Line 122: Line 123: setVmTemplate(latest); Line 124: templateIdToUse = latest.getId(); Line 125: getParameters().getVmStaticData().setVmtGuid(templateIdToUse); Why is this line needed? Isn't the exact same id extracted from the same place when the templateIdToUse is populated for the first time? (the line where it is declared) It looks as if the same value is overriden with same value - why? (perhaps I'm missing something here...) Line 126: } Line 127: } Line 128: Line 129: setVmTemplateId(templateIdToUse); .................................................... File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java Line 495: stat.setPriority(1); Line 496: vm.setStaticData(stat); Line 497: vm.setDynamicData(dynamic); Line 498: vm.setSingleQxlPci(false); Line 499: vm.setTemplateVersion(1); is it testing enough the versions feature if always the version is set to 1 for all test cases? Line 500: return vm; Line 501: } Line 502: Line 503: private AddVmCommand<VmManagementParametersBase> createCommand(VM vm) { .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmTemplateDAODbFacadeImpl.java Line 245: .addValue("vnic_profile_id", vnicProfileId)); Line 246: } Line 247: Line 248: @Override Line 249: public VmTemplate getTemplateWithLatestVersionInChain(Guid id) is "inchain" necessary? what does it represent? (why "latest" is not enough - isn't it clear?) Line 250: { Line 251: return getCallsHandler().executeRead("GetTemplateWithLatestVersionInChain", Line 252: VMTemplateRowMapper.instance, Line 253: getCustomMapSqlParameterSource() -- To view, visit http://gerrit.ovirt.org/22975 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2d1b3ee9cd64d73866762fbf5fe02a468cdd578e Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com> 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
