Maor Lipchuk has posted comments on this change. Change subject: core: Add alias and description for disk meta data ......................................................................
Patch Set 14: (8 comments) http://gerrit.ovirt.org/#/c/34163/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java: Line 79: Map<String, Object> diskDescriptionMap = JsonHelper.jsonToMap(newDiskImage.getDescription()); Line 80: newDiskImage.setDiskAlias((String) diskDescriptionMap.get(ImagesHandler.DISK_ALIAS)); Line 81: newDiskImage.setDiskDescription((String) diskDescriptionMap.get(ImagesHandler.DISK_DESCRIPTION)); Line 82: } catch (IOException e) { Line 83: log.warn("Exception while generating JSON for disk. Exception: '{}'", e); > sorry for missing that, /s/generating JSON for disk/parsing disk descriptio done Line 84: } Line 85: } Line 86: Line 87: // The disk image won't have an interface set on it. Set it to IDE by default. When the http://gerrit.ovirt.org/#/c/34163/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java: Line 855: */ Line 856: public static String getJsonDiskDescription(String diskAlias, String diskDescription) throws IOException { Line 857: Map<String, Object> description = new HashMap<>(); Line 858: description.put(ImagesHandler.DISK_ALIAS, diskAlias); Line 859: description.put(ImagesHandler.DISK_DESCRIPTION, diskDescription != null ? diskDescription : ""); > can you explain why was it changed from the previous patchset? Explained f2f, the description should not be null to preserve compatibility Line 860: return JsonHelper.mapToJson(description, false); Line 861: } http://gerrit.ovirt.org/#/c/34163/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java: Line 180: diskImage.getStorageIds().get(0), diskImage.getStoragePoolId()); Line 181: storageDomainValidator = new StorageDomainValidator(storageDomain); Line 182: if (!validate(storageDomainValidator.isDomainExistAndActive())) { Line 183: return false; Line 184: } > 1. IMO this part is a regression - till now user could update disks regardl will do an audit log as suggested Line 185: if (!validateCanResizeDisk()) { Line 186: return false; Line 187: } Line 188: } Line 356: } Line 357: Line 358: private void performDiskUpdate(final boolean unlockImage) { Line 359: if (shouldPerformMetadataUpdate()) { Line 360: updateMetaDataDescription((DiskImage) getNewDisk()); > The reason that we lock in the db is also for user experience, it's frustra solved Line 361: } Line 362: final Disk disk = getDiskDao().get(getParameters().getDiskId()); Line 363: applyUserChanges(disk); Line 364: Line 398: } Line 399: }); Line 400: } Line 401: Line 402: private boolean shouldPerformMetadataUpdate() { > Please move this method to the images handler, no one will ever remember to I already added a comment in ImagesHandler as described in the previous patchset Line 403: return ((getNewDisk().getDiskStorageType() == DiskStorageType.IMAGE) && (!ObjectUtils.objectsEqual(getOldDisk().getDiskAlias(), Line 404: getNewDisk().getDiskAlias()) || !ObjectUtils.objectsEqual(getOldDisk().getDiskDescription(), Line 405: getNewDisk().getDiskDescription()))); Line 406: } http://gerrit.ovirt.org/#/c/34163/14/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java: Line 129: } Line 130: Line 131: @Test Line 132: public void testJsonNullDiskDescription() throws IOException { Line 133: String jsonDescription = null; > please just define on the next row. done Line 134: jsonDescription = ImagesHandler.getJsonDiskDescription("DiskAlias", null); Line 135: assertTrue("Should be map of disk alias and disk description", Line 136: jsonDescription.equals("{\"DiskDescription\":\"\",\"DiskAlias\":\"DiskAlias\"}")); Line 137: } Line 137: } Line 138: Line 139: @Test Line 140: public void testJsonEmptyDiskDescription() throws IOException { Line 141: String jsonDescription = null; > please just define on the next row. done Line 142: jsonDescription = ImagesHandler.getJsonDiskDescription("DiskAlias", ""); Line 143: assertTrue("Should be map of disk alias and disk description", Line 144: jsonDescription.equals("{\"DiskDescription\":\"\",\"DiskAlias\":\"DiskAlias\"}")); Line 145: } http://gerrit.ovirt.org/#/c/34163/14/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java: Line 620: sd.setAvailableDiskSize(Integer.MAX_VALUE); Line 621: sd.setStatus(StorageDomainStatus.Active); Line 622: when(storageDomainDao.getForStoragePool(any(Guid.class), any(Guid.class))).thenReturn(sd); Line 623: StorageDomainValidator sdValidator = new StorageDomainValidator(sd); Line 624: doReturn(sdValidator).when(command).getStorageDomainValidator(); > with the current implementation you should check it fails when the domain i Done Line 625: } Line 626: Line 627: @Test Line 628: public void testDiskAliasAdnDescriptionMetaDataShouldNotBeUpdated() { -- To view, visit http://gerrit.ovirt.org/34163 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie2642ae7016579ead699509426e01ac2010bd374 Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
