Liron Aravot has posted comments on this change. Change subject: core: Add alias and description for disk meta data ......................................................................
Patch Set 14: (7 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 description as JSON 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? 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 regardless of the domain status and i don't think that we should forbid that - it can hurt users with existing scripts and so on. What i think that should be done is that if the storage domain is not up we will have an audit log to the user (disk metadata wasn't updated on the storage because of the domain status)..and the disk will still be updated in the db. 2. Regardless of 1 (if we do continue with the approach to forbid editing completely if the domain isn't up) The storage domain is already checked in validateCanResizeDisk()- we should check it only once, not twice. regardless we should also decide if we want to grey out the edit button in the UI if we leave as is. Line 185: if (!validateCanResizeDisk()) { Line 186: return false; Line 187: } Line 188: } 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 change it if the description generator there changes. they should be together to minimize that risk - I'd also add a comment on the generating description stating that on change this method should be changed as well. 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. 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. 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 is in invalid status. 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
