Juan Hernandez has posted comments on this change. Change subject: restapi: [WIP!] Phase 5: REST part for console descriptor generator ......................................................................
Patch Set 8: (14 comments) https://gerrit.ovirt.org/#/c/37976/8//COMMIT_MSG Commit Message: Line 21: <port>5900</port> Line 22: ... Line 23: </graphics-framebuffer> Line 24: </graphics-framebuffers> Line 25: </vm> As "graphics-framebuffer" is a sub-collection its contents shouldn't be rendered by default when requesting a VM: GET /vms/{vm:id} <vm> <link href="../vms/{vm:id}/graphicsframebuffers" rel="graphicsframebuffers"/> <!-- Note that there is no other information about graphics devices here. --> </vm> Line 26: Line 27: - /vms/{id}/graphics-framebuffers/spice: Line 28: <graphics-framebuffer> Line 29: <protocol>spice</protocol> Line 23: </graphics-framebuffer> Line 24: </graphics-framebuffers> Line 25: </vm> Line 26: Line 27: - /vms/{id}/graphics-framebuffers/spice: The identifier of a graphics framebuffer should not be "spice" or any other value of a backend enum. It should be the result of hex encoding the identifier of the backend entity. You may choose to hex encode the backend enum. Line 28: <graphics-framebuffer> Line 29: <protocol>spice</protocol> Line 30: <port>5900</port> Line 31: ... https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/ConsoleOptionsHelper.java File backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/ConsoleOptionsHelper.java: Line 21: MultivaluedMap<String, String> consoleOptions = lastSegment.getMatrixParameters(); Line 22: for (Map.Entry<String, List<String>> entry : consoleOptions.entrySet()) { Line 23: updateOptions(options, new Pair<>(entry.getKey(), entry.getValue().get(0))); Line 24: } Line 25: } catch (Exception e) { } Don't swallow the exception, at least send the stack trace to the log. Line 26: Line 27: return options; Line 28: } Line 29: Line 35: break; Line 36: case "release-cursor": Line 37: options.setReleaseCursorHotKey(val); Line 38: break; Line 39: // enough for the demo lol; todo switch (couple of cases) for filling actual console options There should be a "default" branch, and there you should at least report the error in the log. Line 40: } Line 41: } https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/ApiMediaType.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/ApiMediaType.java: Line 26: SLASH No need for a constant just for the slash, in my opinion. https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/GraphicsFramebufferResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/GraphicsFramebufferResource.java: Line 9: Line 10: @GET Line 11: @Actionable Line 12: @Produces({ApiMediaType.APPLICATION_X_VIRT_VIEWER}) Line 13: public Response descriptor(); So this resource will only support the .vv content type? What about XML and JSON? Line 14: https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/GraphicsFramebuffersResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/GraphicsFramebuffersResource.java: Line 8: Line 9: @Produces({ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML}) Line 10: public interface GraphicsFramebuffersResource { Line 11: Line 12: @Path("{protocol}") The name of the parameter should be "id", like in all other resources. Line 13: public GraphicsFramebufferResource getGraphicsSubresource(@PathParam("protocol") GraphicsType protocol); Line 14: Line 9: @Produces({ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML}) Line 10: public interface GraphicsFramebuffersResource { Line 11: Line 12: @Path("{protocol}") Line 13: public GraphicsFramebufferResource getGraphicsSubresource(@PathParam("protocol") GraphicsType protocol); Same here. And the type must be a string, not a enum, specially not a backend enum. Line 14: https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/VmResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/VmResource.java: Line 170: Line 171: @Path("sessions") Line 172: public VmSessionsResource getVmSessionsResource(); Line 173: Line 174: @Path("graphics-framebuffers") Don't use any separator in URL segments. Line 175: public GraphicsFramebuffersResource graphicsFramebuffersResource(); Line 176: https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3811: </xs:extension> Line 3812: </xs:complexContent> Line 3813: </xs:complexType> Line 3814: Line 3815: <!--todo rename lol--> Whatever names you choose they should use _ as separator, not -. Line 3816: <xs:element name="graphics-framebuffers" type="GraphicsFramebuffers"/> Line 3817: <xs:element name="graphics-framebuffer" type="GraphicsFramebuffer"/> Line 3818: Line 3819: <xs:complexType name="GraphicsFramebuffers"> Line 3823: maxOccurs Should be "unbounded". The fact that there are two currently is an implementation detail. Line 3834: Line 3835: <xs:complexType name="GraphicsFramebuffer"> Line 3836: <xs:complexContent> Line 3837: <xs:extension base="Display"> Line 3838: <!--todo basically same fields as in Display Field. For BC, Display can't be removed --> If display will be eventually removed (in 4.x) then it is better to avoid a dependency here. Just duplicate the required fields. Line 3839: </xs:extension> Line 3840: </xs:complexContent> Line 3841: </xs:complexType> Line 3842: https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml File backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml: Line 538: protocol Should be "graphics-framebuffer:id", not "protocol", otherwise the generators of the SDKs won't work correctly. Either that or use "protocols" instead of "graphics-framebuffers". https://gerrit.ovirt.org/#/c/37976/8/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendGraphicsFramebuffersResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendGraphicsFramebuffersResource.java: Line 15: this.vmId = Guid.createGuidFromString(vmId); Line 16: } Line 17: Line 18: @Override Line 19: public GraphicsFramebufferResource getGraphicsSubresource(@PathParam("protocol") GraphicsType protocol) { The @ParamAnnotation here isn't needed, put it only in the interface. And the type must be a string, not a backend enum. Line 20: BackendGraphicsFramebufferResource backendGraphicsResource = new BackendGraphicsFramebufferResource(protocol, vmId); Line 21: inject(backendGraphicsResource); Line 22: return backendGraphicsResource; // todo Line 23: } -- To view, visit https://gerrit.ovirt.org/37976 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08814006b413a7f213561e1884166af4d0d6c8ae Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[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
