Allon Mureinik has posted comments on this change. Change subject: core: persist updated vm/template ovf config ......................................................................
Patch Set 6: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/24178/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmAndTemplatesGenerationsDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmAndTemplatesGenerationsDbFacadeImpl.java: Line 9: import org.ovirt.engine.core.compat.Guid; Line 10: import org.springframework.jdbc.core.RowMapper; Line 11: Line 12: public class VmAndTemplatesGenerationsDbFacadeImpl extends BaseDAODbFacade implements VmAndTemplatesGenerationsDAO { Line 13: private static final String ovfSeparator = "ENDOVF"; On second thought, this is extremely breakable. Nothing is stopping you from having this in a VM/disk name, which would shatter the parsing completely. How about using a non-printable/binary/non-ascii string? Line 14: Line 15: @Override Line 16: public void updateOvfGenerations(List<Guid> ids, List<Long> values, List<String> ovfData) { Line 17: getCallsHandler().executeModification("UpdateOvfGenerations", getCustomMapSqlParameterSource() http://gerrit.ovirt.org/#/c/24178/6/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmAndTemplatesGenerationsDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmAndTemplatesGenerationsDaoTest.java: Line 73: } Line 74: Line 75: @Test Line 76: public void testGetVmTemplatesIdsForOvfUpdateOneTemplate() { Line 77: vmAndTemplatesGenerationsDAO.updateOvfGenerations(Collections.singletonList(FixturesTool.VM_TEMPLATE_RHEL5), Collections.singletonList(Long.valueOf(0)), Arrays.asList("a")); Please use Collections.singletonList instead of Arrays.asList - there isn't a big difference here, but it'll stop FB from nagging. Line 78: Line 79: List<Guid> guids = vmAndTemplatesGenerationsDAO.getVmTemplatesIdsForOvfUpdate(FixturesTool.STORAGE_POOL_RHEL6_ISCSI_OTHER); Line 80: assertEquals("one template should need ovf update", 1, guids.size()); Line 81: assertEquals("wrong template returned as in need for ovf update", guids.get(0), FixturesTool.VM_TEMPLATE_RHEL5); Line 146: } Line 147: Line 148: @Test Line 149: public void testGetVmssIdsForOvfUpdateOneVm() { Line 150: vmAndTemplatesGenerationsDAO.updateOvfGenerations(Collections.singletonList(FixturesTool.VM_RHEL5_POOL_50), Collections.singletonList(Long.valueOf(0)), Arrays.asList("a")); same. Line 151: Line 152: List<Guid> guids = vmAndTemplatesGenerationsDAO.getVmsIdsForOvfUpdate(FixturesTool.STORAGE_POOL_RHEL6_ISCSI_OTHER); Line 153: assertEquals("one vm should need ovf update", 1, guids.size()); Line 154: assertEquals("wrong vm returned as in need for ovf update", guids.get(0), FixturesTool.VM_RHEL5_POOL_50); http://gerrit.ovirt.org/#/c/24178/6/packaging/dbscripts/vms_sp.sql File packaging/dbscripts/vms_sp.sql: Line 39: AS $procedure$ Line 40: BEGIN Line 41: RETURN QUERY SELECT * Line 42: FROM vm_ovf_generations ovf Line 43: WHERE ovf.vm_guid IN (SELECT * FROM fnSplitterUuid(v_ids)); Can't we use WHERE ovf.vm_guid IN fnSplitterUuid(v_ids) Line 44: END; $procedure$ Line 45: LANGUAGE plpgsql; Line 46: Line 47: -- To view, visit http://gerrit.ovirt.org/24178 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic891429a2a7d055fbd1927e878ae7e0f8b7c9fd9 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@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
