Michael Pasternak has posted comments on this change.

Change subject: restapi: API should expose hypervisor version (#829625)
......................................................................


Patch Set 4: I would prefer that you didn't submit this

(5 inline comments)

....................................................
File 
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendHostsResourceTest.java
Line 277:                 control.createMock(RpmVersion.class), index);
Line 278:     }
Line 279: 
Line 280:     static VDS setUpEntityExpectations(VDS entity, RpmVersion 
version, int index) {
Line 281:         return setUpEntityExpectations(entity, null, version, index);
your code should work regardless 'version' being set or not ...
Line 282:     }
Line 283: 
Line 284:     static VDS setUpEntityExpectations(VDS entity, VdsStatistics 
statistics, RpmVersion version, int index) {
Line 285:         expect(entity.getId()).andReturn(GUIDS[index]).anyTimes();


....................................................
File 
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendResourceTest.java
Line 115:     }
Line 116: 
Line 117: 
Line 118:     protected VDS getEntity(int index) {
Line 119:         VDS vds = 
setUpEntityExpectations(control.createMock(VDS.class), 
control.createMock(RpmVersion.class), index);
i'd create rpmversion in dedicated test rather than forcing all other tests 
defining it

(!NULL check in mapper will do the trick for you)
Line 120:         VdsStatic vdsStatic = control.createMock(VdsStatic.class);
Line 121:         expect(vdsStatic.getId()).andReturn(GUIDS[2]).anyTimes();
Line 122:         expect(vds.getStaticData()).andReturn(vdsStatic).anyTimes();
Line 123:         return vds;


....................................................
File 
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainsResourceTest.java
Line 155: 
Line 156:     @Test
Line 157:     public void testRemoveWithHostName() throws Exception {
Line 158:         setUpGetEntityExpectations();
Line 159:         VDS host = 
BackendHostsResourceTest.setUpEntityExpectations(control.createMock(VDS.class), 
control.createMock(RpmVersion.class), 1);
same here (no need to define the version)
Line 160:         setUpGetEntityExpectations("Hosts: name=" + NAMES[1],
Line 161:                                    SearchType.VDS,
Line 162:                                    host);
Line 163: 


....................................................
File 
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmResourceTest.java
Line 695: 
Line 696:     protected void setUpGetHostIdExpectations(int idx) throws 
Exception {
Line 697:         VDS host = 
BackendHostsResourceTest.setUpEntityExpectations(control.createMock(VDS.class),
Line 698:                 control.createMock(RpmVersion.class),
Line 699:                 idx);
same here (no need to define the version)
Line 700:         setUpGetEntityExpectations(VdcQueryType.GetVdsByName,
Line 701:                                    GetVdsByNameParameters.class,
Line 702:                                    new String[] { "Name" },
Line 703:                                    new Object[] { NAMES[idx] },


....................................................
File 
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/HostMapper.java
Line 140:         sm.setPriority(entity.getVdsSpmPriority());
Line 141:         sm.setValue(entity.getspm_status() == VdsSpmStatus.SPM);
Line 142:         model.setStorageManager(sm);
Line 143:         Version version = new Version();
Line 144:         version.setMajor(entity.getVersion().getMajor());
what if vdsm is not installed on the host? (i.e entity.getVersion() returns 
null)

"entity.getVersion().getMajor()" will throw NPE?
Line 145:         version.setMinor(entity.getVersion().getMinor());
Line 146:         version.setRevision(entity.getVersion().getRevision());
Line 147:         version.setBuild(entity.getVersion().getBuild());
Line 148:         version.setFullVersion(entity.getVersion().getRpmName());


--
To view, visit http://gerrit.ovirt.org/9220
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id8d3d090ae4c0ba489f3c7484d5c232c55f3c059
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to