Juan Hernandez has posted comments on this change. Change subject: engine: Enable read of current CD via REST ......................................................................
Patch Set 5: Code-Review+1 (2 comments) The RESTAPI part looks good to me. However, if possible, I would suggest to add to BackendCdRomResourceTest tests to verify that the correct CD is selected when the "current" parameter is given. http://gerrit.ovirt.org/#/c/20368/5/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendCdRomResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendCdRomResource.java: Line 42: } Line 43: Line 44: @Override Line 45: public CdRom get() { Line 46: if (QueryHelper.hasMatrixParam(getUriInfo(), CURRENT_CONSTRAINT_PARAMETER)) { We should check here the value of the parameter, not just its existence. If the URL is "...;current=false" then we should return the persistent CDROM, not the current. Line 47: VM vm = collection.lookupEntity(guid); Line 48: if (vm == null) { Line 49: return notFound(); Line 50: } Line 51: // if the CD has changed during the run of VM Line 52: if (vm.getCurrentCd() != null) { Line 53: // change the iso path so the result of 'map' will contain current cd instead of the Line 54: // persistent configuration Line 55: vm.setIsoPath(vm.getCurrentCd()); > but when the vm will be down this will change again, right? The user can still know what is the persistent CDROM, just omiting the "current" parameter or setting its value to "false": /api/vms/{vm:id}/cdroms/{cdrom:id};current=true -> The current CD. /api/vms/{vm:id}/cdroms/{cdrom:id};current=false -> The persistent CD. /api/vms/{vm:id}/cdroms/{cdrom:id} -> The persistent CD (this is needed for backwards compatibility) Line 56: } Line 57: return addLinks(populate(map(vm), vm)); Line 58: } else { Line 59: return super.get(); -- To view, visit http://gerrit.ovirt.org/20368 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd6c80984143b7d953b6d76b9863465aabce0917 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Michael Pasternak <[email protected]> Gerrit-Reviewer: Omer Frenkel <[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
