Tomas Jelinek has posted comments on this change. Change subject: restapi: Phase 5: Add graphics console representation ......................................................................
Patch Set 6: (15 comments) https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3013: </xs:complexType> Line 3014: Line 3015: <xs:element name="graphics_consoles" type="GraphicsConsoles" /> Line 3016: Line 3017: <xs:complexType name="GraphicsConsoles"> > This type should extend "BaseResources". Done Line 3018: <xs:sequence> Line 3019: <xs:element ref="graphics_console" minOccurs="0" maxOccurs="unbounded" /> Line 3020: </xs:sequence> Line 3021: </xs:complexType> Line 3015: <xs:element name="graphics_consoles" type="GraphicsConsoles" /> Line 3016: Line 3017: <xs:complexType name="GraphicsConsoles"> Line 3018: <xs:sequence> Line 3019: <xs:element ref="graphics_console" minOccurs="0" maxOccurs="unbounded" /> > There should be an annotation for this element to specify that the correspo Done Line 3020: </xs:sequence> Line 3021: </xs:complexType> Line 3022: Line 3023: <xs:element name="graphics_console" type="GraphicsConsole" /> Line 3021: </xs:complexType> Line 3022: Line 3023: <xs:element name="graphics_console" type="GraphicsConsole" /> Line 3024: Line 3025: <xs:complexType name="GraphicsConsole"> > This type should extends "BaseResource", and then we don't need to explicit Done Line 3026: <xs:sequence> Line 3027: <xs:element name="protocol" type="xs:string" /> Line 3028: <xs:element name="port" type="xs:int" /> Line 3029: <xs:element name="tls_port" type="xs:int" /> Line 3027: <xs:element name="protocol" type="xs:string" /> Line 3028: <xs:element name="port" type="xs:int" /> Line 3029: <xs:element name="tls_port" type="xs:int" /> Line 3030: <xs:element name="address" type="xs:string" /> Line 3031: </xs:sequence> > Be specific with the "minOccurs" and "maxOccurs" of the elements inside the Done Line 3032: <xs:attribute name="id" type="xs:string" /> Line 3033: </xs:complexType> Line 3034: Line 3035: <xs:complexType name="Ticket"> Line 3029: <xs:element name="tls_port" type="xs:int" /> Line 3030: <xs:element name="address" type="xs:string" /> Line 3031: </xs:sequence> Line 3032: <xs:attribute name="id" type="xs:string" /> Line 3033: </xs:complexType> > Before merging remember to use two spaces per level. Done Line 3034: Line 3035: <xs:complexType name="Ticket"> Line 3036: <xs:sequence> Line 3037: <xs:element name="value" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3289: <xs:element ref="migration" minOccurs="0" maxOccurs="1" /> Line 3290: <xs:element name="custom_properties" type="CustomProperties" minOccurs="0" maxOccurs="1"/> Line 3291: <xs:element name="custom_emulated_machine" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3292: <xs:element name="custom_cpu_model" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3293: <xs:element name="graphics_consoles" type="GraphicsConsoles"/> > Be explicit with minOccurs and maxOccurs. Done Line 3294: </xs:sequence> Line 3295: </xs:extension> Line 3296: </xs:complexContent> Line 3297: </xs:complexType> https://gerrit.ovirt.org/#/c/39057/6/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 556: body: Line 557: parameterType: null Line 558: signatures: [] Line 559: urlparams: {} Line 560: # str: {context: matrix, type: 'xs:string', value: 'todo', required: true} > Don't specify the "get" link twice, as that will break the generators of th Done Line 561: headers: Line 562: Content-Type: {value: application/x-virt-viewer, required: true} Line 563: Filter: {value: true|false, required: false} Line 564: - name: /vms/{vm:id}/logon|rel=logon https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsoleResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsoleResource.java: Line 33: if (consoleId.equals(graphicsConsole.getId())) { Line 34: return graphicsConsole; Line 35: } Line 36: } Line 37: return null; > Returning null will likely generate an ugly stack trace in the result. Chec done - throwing an WebApplicationException which is than properly translated to nice response Line 38: } Line 39: Line 40: @Override Line 41: public Response getConsoleDescriptor() { Line 41: public Response getConsoleDescriptor() { Line 42: GraphicsType graphicsType = GraphicsTypeHelper.fromGraphicsTypeHexa(consoleId); Line 43: Line 44: if (graphicsType == null) { Line 45: throw new NullPointerException("something is wrong"); > I'd suggest to use a more descriptive message, like "can't find graphics ty replaced by validateEnums Line 46: } Line 47: Line 48: ConsoleOptions consoleOptions = new ConsoleOptions(graphicsType); Line 49: consoleOptions.setVmId(vmGuid); Line 48: ConsoleOptions consoleOptions = new ConsoleOptions(graphicsType); Line 49: consoleOptions.setVmId(vmGuid); Line 50: ConsoleOptions configuredOptions = runQuery(VdcQueryType.ConfigureConsoleOptions, Line 51: new ConfigureConsoleOptionsParams(consoleOptions, true)).getReturnValue(); // fill required data from BE Line 52: // todo adjust console opts based on query/matrix/whatever params > Will that be part of a different patch? It will likely not be supported at all. Currently the patch returns the .vv file which will be than used to open the console. This comment was about customizing the .vv file - e.g. tell the API to return a .vv file which will have such and such tls-port for example. But this is Id say not something the REST API should provide - it should generate the .vv file based for the VM and not an arbitrary .vv file I tell it. In worst case I can anyway post-process the returned .vv file on the client side. Line 53: Line 54: String descriptor = runQuery(VdcQueryType.GetConsoleDescriptorFile, new ConsoleOptionsParams(configuredOptions)) Line 55: .getReturnValue(); Line 56: https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsolesResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsolesResource.java: Line 8: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 9: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 10: import org.ovirt.engine.core.compat.Guid; Line 11: Line 12: public class BackendVmGraphicsConsolesResource // backendVmgraphics... > What is the meaning of this comment? Done Line 13: extends BackendResource Line 14: implements VmGraphicsConsolesResource { Line 15: Line 16: private final Guid guid; Line 9: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 10: import org.ovirt.engine.core.compat.Guid; Line 11: Line 12: public class BackendVmGraphicsConsolesResource // backendVmgraphics... Line 13: extends BackendResource > This should extend AbstractBackendCollectionResource<GraphicsConsoles, VM> Done Line 14: implements VmGraphicsConsolesResource { Line 15: Line 16: private final Guid guid; Line 17: Line 12: public class BackendVmGraphicsConsolesResource // backendVmgraphics... Line 13: extends BackendResource Line 14: implements VmGraphicsConsolesResource { Line 15: Line 16: private final Guid guid; > That base class provides a "parentId" attribute, so this won't be needed. I think not in AbstractBackendCollectionResource. Did you mean some other class to extend? Line 17: Line 18: public BackendVmGraphicsConsolesResource(Guid guid) { Line 19: this.guid = guid; Line 20: } https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GraphicsTypeHelper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GraphicsTypeHelper.java: Line 5: Line 6: public class GraphicsTypeHelper { Line 7: Line 8: private static final String SPICE_STRING = "SPICE"; Line 9: private static final String VNC_STRING = "VNC"; > The usual way to do this in the RESTAPI is to define a "GraphicsType" enum, Done Line 10: Line 11: Line 12: public static String graphicsTypeToString(GraphicsType graphicsType) { Line 13: if (graphicsType != null) { https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java: Line 565: } Line 566: Line 567: @Mapping(from = org.ovirt.engine.core.common.businessentities.VM.class, to = GraphicsConsoles.class) Line 568: public static GraphicsConsoles map(org.ovirt.engine.core.common.businessentities.VM vm, GraphicsConsoles template) { Line 569: template = template != null ? template : new GraphicsConsoles(); > Avoid returning "template" here, it is confusing. Create a "model" variable Done Line 570: Line 571: for (Map.Entry<GraphicsType, GraphicsInfo> graphicsInfo : vm.getGraphicsInfos().entrySet()) { Line 572: GraphicsConsole graphics = new GraphicsConsole(); Line 573: String graphicsTypeString = GraphicsTypeHelper.graphicsTypeToString(graphicsInfo.getKey()); -- To view, visit https://gerrit.ovirt.org/39057 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7dbc63ac49bfba5637c38b84b373d78f348c7872 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
