Michael Pasternak has posted comments on this change.
Change subject: restapi : vm/template Import candidates should have /disks
(#829672)
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(6 inline comments)
please add dedicated test classes for the new collections/resources
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendExportDomainDiskResource.java
Line 27:
Line 28: @Override
Line 29: public Disk get() {
Line 30: Disks disks = parent.list();
Line 31: for (Disk disk : disks.getDisks()) {
i'd move this to the inheriting classes (making this method abstract) where you
have map of <guid, disk>, so you can fetch disk in more efficient way - like in
~O(1) (even if amount of disks is not that big) instead of
1. fetch all disks
2. map each to api object
3. loop over and find the current or throw notFound()
Line 32: if (disk.getId().equals(diskId)) {
Line 33: return disk;
Line 34: }
Line 35: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendExportDomainVmDisksResource.java
Line 15:
Line 16: @Override
Line 17: protected Set<Guid> getDiskIds() {
Line 18: if (getVM() == null) {
Line 19: System.out.println("VM is null");
debug prints?
Line 20: }
Line 21: if (getVM().getDiskMap() == null) {
Line 22: System.out.println("DiskMap is null");
Line 23: }
Line 18: if (getVM() == null) {
Line 19: System.out.println("VM is null");
Line 20: }
Line 21: if (getVM().getDiskMap() == null) {
Line 22: System.out.println("DiskMap is null");
debug prints?
Line 23: }
Line 24: return getVM().getDiskMap().keySet();
Line 25: }
Line 26:
Line 20: }
Line 21: if (getVM().getDiskMap() == null) {
Line 22: System.out.println("DiskMap is null");
Line 23: }
Line 24: return getVM().getDiskMap().keySet();
if getVM().getDiskMap() == null or getVM() == null, you have NPE here?
is this ^ possible?
Line 25: }
Line 26:
Line 27: @Override
Line 28: protected org.ovirt.engine.core.common.businessentities.Disk
getDisk(Guid id) {
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainTemplateResourceTest.java
Line 82: }
Line 83: }
Line 84:
Line 85: @Test
Line 86: public void testGetDisks() throws Exception {
please move this test to dedicated BackendExportDomainVmDiskResourceTest.java
Line 87:
setUpGetStorageDomainExpectations(StorageDomainType.ImportExport);
Line 88: setUpGetEntityExpectations(StorageDomainType.ImportExport,
STORAGE_DOMAIN_ID);
Line 89: setUriInfo(setUpBasicUriExpectations());
Line 90: control.replay();
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainVmResourceTest.java
Line 85: }
Line 86: }
Line 87:
Line 88: @Test
Line 89: public void testGetDisks() throws Exception {
please move this test to dedicated
BackendExportDomainTemplateDisksResourceTest.java
Line 90:
setUpGetStorageDomainExpectations(StorageDomainType.ImportExport);
Line 91: setUpGetEntityExpectations(StorageDomainType.ImportExport,
STORAGE_DOMAIN_ID);
Line 92: setUriInfo(setUpBasicUriExpectations());
Line 93: control.replay();
--
To view, visit http://gerrit.ovirt.org/11126
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I83fd4b046f72f73bb516615137522ac40028a7f0
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
