Liron Aravot has posted comments on this change. Change subject: restapi: Enabling disk information editing when adding a new template. ......................................................................
Patch Set 4: (5 comments) http://gerrit.ovirt.org/#/c/37808/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Disk.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Disk.java: Line 139: public void setOvfStore(boolean ovfStore) { Line 140: this.ovfStore = ovfStore; Line 141: } Line 142: Line 143: public boolean isDiskImage() { I'm not really in favor of having this method here, it means that the parent class is aware of it's possible types (which are the sons). you can have it in the DiskStorageType if you want to avoid the equality check..but i don't really like that either :) if (diskImage.getDiskStorageType().Image()) Line 144: return getDiskStorageType() == DiskStorageType.IMAGE; Line 145: } Line 146: Line 147: /** http://gerrit.ovirt.org/#/c/37808/4/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendTemplatesResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendTemplatesResource.java: Line 140: private VDSGroup lookupCluster(Guid id) { Line 141: return getEntity(VDSGroup.class, VdcQueryType.GetVdsGroupByVdsGroupId, new IdQueryParameters(id), "GetVdsGroupByVdsGroupId"); Line 142: } Line 143: Line 144: protected HashMap<Guid, DiskImage> getDiskToDestinationMap(VM vm, Guid storageDomainId, boolean isDomainSet) { till now this map was used only to reflect the destination of the disk - so the name of this method should be changed. Line 145: HashMap<Guid, DiskImage> diskToDestinationMap = null; Line 146: if (vm.isSetDisks() && vm.getDisks().isSetDisks()) { Line 147: diskToDestinationMap = new HashMap<Guid, DiskImage>(); Line 148: Map<Guid, org.ovirt.engine.core.common.businessentities.Disk> vmSourceDisks = queryVMDiskList(vm); Line 144: protected HashMap<Guid, DiskImage> getDiskToDestinationMap(VM vm, Guid storageDomainId, boolean isDomainSet) { Line 145: HashMap<Guid, DiskImage> diskToDestinationMap = null; Line 146: if (vm.isSetDisks() && vm.getDisks().isSetDisks()) { Line 147: diskToDestinationMap = new HashMap<Guid, DiskImage>(); Line 148: Map<Guid, org.ovirt.engine.core.common.businessentities.Disk> vmSourceDisks = queryVMDiskList(vm); why do we need the query? only the disk alias and disk profile are considered in the command if given, you can just set them to the passed disk in the old code. am i missing something? furthermore, you should specify in the rsdl_metadata.yaml file what can be edited in the disk. Line 149: Line 150: for (Disk disk : vm.getDisks().getDisks()) { Line 151: Guid currDiskID = asGuid(disk.getId()); Line 152: if (disk.isSetId() && vmSourceDisks.containsKey(currDiskID)) { Line 152: if (disk.isSetId() && vmSourceDisks.containsKey(currDiskID)) { Line 153: org.ovirt.engine.core.common.businessentities.Disk sourceDisk = vmSourceDisks.get(currDiskID); Line 154: Line 155: // VM template can only have disk images Line 156: if (sourceDisk.isDiskImage()) { I'd change the if to eliminate the indentation.. if (!isDiskImage) continue Line 157: DiskImage destinationDisk = (DiskImage) DiskMapper.map(disk, sourceDisk); Line 158: Line 159: if (disk.isSetStorageDomains() && disk.getStorageDomains().isSetStorageDomains() ) { Line 160: List<org.ovirt.engine.api.model.StorageDomain> diskStorageDomains = Line 161: disk.getStorageDomains().getStorageDomains(); Line 162: Line 163: if (!diskStorageDomains.isEmpty() && diskStorageDomains.get(0).isSetId()) { Line 164: destinationDisk.setStorageIds(new ArrayList<Guid>(1)); Line 165: Guid newStorageDomainId = if we are already touching this, we check if the domain is set along the way and then ignore it if domain was passed? so why not avoid all that checking beforehand? perhaps i missed something. Line 166: isDomainSet ? storageDomainId : asGuid(diskStorageDomains.get(0).getId()); Line 167: destinationDisk.getStorageIds().add(newStorageDomainId); Line 168: } Line 169: diskToDestinationMap.put(destinationDisk.getId(), destinationDisk); -- To view, visit http://gerrit.ovirt.org/37808 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0baf4e5bd7233fbc4eab2f1f671b8b15e08ba03 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
