Amit Aviram has posted comments on this change. Change subject: restapi: Enabling disk information editing when adding a new template. ......................................................................
Patch Set 4: (6 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 paren Done- Moving this logic to the BackendTemplatesResource class. 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 Done 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? The fact that rsdl_metadata.yaml does not show alias as an option is a bug itself, it will be fixed in another patch. 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.. Done 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 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.. Done 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 wa The thing here is that it is possible to provide the storage domain in 2 ways: 1. like this: <template> <vm id='...'> <storage_domains> <storage_domain id='ABC'> </storage_domain> </storage_domains> <disks> <disk id='...'> </disk> </disks> </vm> </template> --> in this case isDomainSet is true, storageDomainId is ABC 2. like this: <template> <vm id='...'> <disks> <disk id='...'> <storage_domains> <storage_domain id='DEG'> </storage_domain> </storage_domains> </disk> </disks> </vm> </template> --> in this case isDomainSet is false, storageDomainId is null in both cases isSetStorageDomains is true, but this line chooses the ID from the right place.. 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
