Juan Hernandez has posted comments on this change. Change subject: Api: Missing domain field on VM\Template object ......................................................................
Patch Set 3: (15 comments) Looks good. Some comments about redundant things and names of variables. http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetVmsInitParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetVmsInitParameters.java: Line 3: import java.util.List; Line 4: Line 5: import org.ovirt.engine.core.compat.Guid; Line 6: Line 7: public class GetVmsInitParameters extends VdcQueryParametersBase { There will probably be more commands requiring only a list of ids, so I think it is better to name this just "IdsQueryParameters", similar to the existing "IdQueryParameters". Line 8: private static final long serialVersionUID = 575294540991590541L; Line 9: Line 10: private List<Guid> ids; Line 11: http://gerrit.ovirt.org/#/c/25154/3/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 147: return performAction(VdcActionType.RemoveVmTemplate, new VmTemplateParametersBase(asGuid(id))); Line 148: } Line 149: Line 150: protected Templates mapCollection(List<VmTemplate> entities) { Line 151: // Fill VmInit for entities - the search query no join the VmInit to Tempaltes s/Tempaltes/Templates/ Line 152: GetVmsInitParameters params = new GetVmsInitParameters(); Line 153: List<Guid> ids = Entities.getIds(entities); Line 154: params.setId(ids); Line 155: VdcQueryReturnValue queryReturnValue = runQuery(VdcQueryType.GetVmsInit, params); Line 157: List<VmInit> vmInits = queryReturnValue.getReturnValue(); Line 158: Map<Guid, VmInit> initMap = Entities.businessEntitiesById(vmInits); Line 159: for (VmTemplate template : entities) { Line 160: template.setVmInit(initMap.get(template.getId())); Line 161: } Templates and VmInit have the same ids? Line 162: } Line 163: Line 164: Templates collection = new Templates(); Line 165: for (VmTemplate entity : entities) { http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendExportDomainDiskResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendExportDomainDiskResourceTest.java: Line 177: return ret; Line 178: } Line 179: Line 180: protected VmTemplate getVmTemplateEntity(int index) { Line 181: VmTemplate vm = setUpEntityExpectations(control.createMock(VmTemplate.class), null, index); This extra parameter shouldn't be needed. Line 182: org.easymock.EasyMock.expect(vm.getDiskTemplateMap()).andReturn(getDiskMap()).anyTimes(); Line 183: return vm; Line 184: } Line 185: http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendExportDomainDisksResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendExportDomainDisksResourceTest.java: Line 253: return ret; Line 254: } Line 255: Line 256: protected VmTemplate getVmTemplateEntity(int index) { Line 257: VmTemplate vm = setUpEntityExpectations(control.createMock(VmTemplate.class), null, index); Same, not needed. Line 258: org.easymock.EasyMock.expect(vm.getDiskTemplateMap()).andReturn(getDiskMap()).anyTimes(); Line 259: return vm; Line 260: } Line 261: http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainTemplateResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainTemplateResourceTest.java: Line 312: } Line 313: Line 314: @Override Line 315: protected VmTemplate getEntity(int index) { Line 316: return setUpEntityExpectations(control.createMock(VmTemplate.class), null, index); Same, not needed. Line 317: } Line 318: Line 319: protected HashMap<VmTemplate, List<DiskImage>> setUpTemplates(boolean notFound) { Line 320: HashMap<VmTemplate, List<DiskImage>> ret = new HashMap<VmTemplate, List<DiskImage>>(); http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainTemplatesResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainTemplatesResourceTest.java: Line 119: } Line 120: Line 121: @Override Line 122: protected VmTemplate getEntity(int index) { Line 123: return setUpEntityExpectations(control.createMock(VmTemplate.class), null, index); Same, not needed. Line 124: } Line 125: Line 126: protected List<VmTemplate> setUpTemplates() { Line 127: List<VmTemplate> ret = new ArrayList<VmTemplate>(); http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendTemplateResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendTemplateResourceTest.java: Line 288: } Line 289: Line 290: @Override Line 291: protected VmTemplate getEntity(int index) { Line 292: return setUpEntityExpectations(control.createMock(VmTemplate.class), null, index); Same, not needed. Line 293: } Line 294: Line 295: protected void setUpGetEntityExpectations(int times) throws Exception { Line 296: setUpGetEntityExpectations(times, false); http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendTemplatesResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendTemplatesResourceTest.java: Line 586: protected VmTemplate getEntity(int index) { Line 587: return setUpEntityExpectations(control.createMock(VmTemplate.class), null, index); Line 588: } Line 589: Line 590: static VmTemplate setUpEntityExpectations(VmTemplate entity, VmInit init, int index) { This new paramer isn't needed. It was needed in a previous patch set, because the new query returned VmTemplate, not VmInit. Line 591: expect(entity.getId()).andReturn(GUIDS[index]).anyTimes(); Line 592: expect(entity.getVdsGroupId()).andReturn(GUIDS[2]).anyTimes(); Line 593: expect(entity.getName()).andReturn(NAMES[index]).anyTimes(); Line 594: expect(entity.getDescription()).andReturn(DESCRIPTIONS[index]).anyTimes(); Line 593: expect(entity.getName()).andReturn(NAMES[index]).anyTimes(); Line 594: expect(entity.getDescription()).andReturn(DESCRIPTIONS[index]).anyTimes(); Line 595: expect(entity.getNumOfCpus()).andReturn(8).anyTimes(); Line 596: expect(entity.getNumOfSockets()).andReturn(2).anyTimes(); Line 597: expect(entity.getVmInit()).andReturn(init).anyTimes(); Not needed. Line 598: if(index == 2) { Line 599: expect(entity.getTemplateVersionName()).andReturn(VERSION_NAME).anyTimes(); Line 600: expect(entity.getTemplateVersionNumber()).andReturn(2).anyTimes(); Line 601: expect(entity.getBaseTemplateId()).andReturn(GUIDS[1]).anyTimes(); Line 715: } Line 716: Line 717: @Override Line 718: protected void setUpQueryExpectations(String query, Object failure) throws Exception { Line 719: // If the query to retrieve the virtual machines succeeds, then we will run another query to add the s/virtual machines/templates/ Line 720: // initialization information: Line 721: if (failure == null) { Line 722: setUpEntityQueryExpectations( Line 723: VdcQueryType.GetVmsInit, Line 732: super.setUpQueryExpectations(query, failure); Line 733: } Line 734: Line 735: private List<VmInit> setUpVmInit() { Line 736: List<VmInit> templates = new ArrayList<>(NAMES.length); Rename this variable to "vmInits" or something similar. Line 737: for (int i = 0; i < NAMES.length; i++) { Line 738: VmInit vmInit = control.createMock(VmInit.class); Line 739: VmInit init = control.createMock(VmInit.class); Line 740: templates.add(vmInit); Line 735: private List<VmInit> setUpVmInit() { Line 736: List<VmInit> templates = new ArrayList<>(NAMES.length); Line 737: for (int i = 0; i < NAMES.length; i++) { Line 738: VmInit vmInit = control.createMock(VmInit.class); Line 739: VmInit init = control.createMock(VmInit.class); This ^line is redundant, the "init" variable is never used. Line 740: templates.add(vmInit); Line 741: } Line 742: return templates; Line 743: } http://gerrit.ovirt.org/#/c/25154/3/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmsResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmsResourceTest.java: Line 1507: super.setUpQueryExpectations(query, failure); Line 1508: } Line 1509: Line 1510: private List<VmInit> setUpVmInit() { Line 1511: List<VmInit> templates = new ArrayList<>(NAMES.length); Rename this variable to "vmInits", or something similar. Line 1512: for (int i = 0; i < NAMES.length; i++) { Line 1513: VmInit vmInit = control.createMock(VmInit.class); Line 1514: VmInit init = control.createMock(VmInit.class); Line 1515: templates.add(vmInit); Line 1510: private List<VmInit> setUpVmInit() { Line 1511: List<VmInit> templates = new ArrayList<>(NAMES.length); Line 1512: for (int i = 0; i < NAMES.length; i++) { Line 1513: VmInit vmInit = control.createMock(VmInit.class); Line 1514: VmInit init = control.createMock(VmInit.class); The "init" variable is never used. Line 1515: templates.add(vmInit); Line 1516: } Line 1517: return templates; Line 1518: } -- To view, visit http://gerrit.ovirt.org/25154 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf43cb1a0d25d89ef68f21bc47b82c2cd6fdf19d Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Shahar Havivi <[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
