Juan Hernandez has posted comments on this change.
Change subject: engine: Change DisplayType semantics
......................................................................
Patch Set 32:
(4 comments)
Will you add support for the new semantics in a later patch? If so please
consider adding a /vm/{vm:id}/displays sub-collection that allows managing the
multiple displays of the virtual machine.
http://gerrit.ovirt.org/#/c/30837/32/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 499: ? null
Line 500: : entity.getGraphicsInfos().get(graphicsType);
Line 501:
Line 502: model.setDisplay(new Display());
Line 503: model.getDisplay().setType(graphicsTypeString);
The values of a backend enum (GraphicsType) should never be directly copied
into a RESTAPI model, as that means that a change in the backend will
inadvertently cause a compatibility problem in the RESTAPI.
You should first translate the graphics type into a instance of DisplayType
(org.ovirt.engine.api.model.DisplayType) and then convert that to String.
Line 504: model.getDisplay().setAddress(graphicsInfo == null ? null
: graphicsInfo.getIp());
Line 505: Integer displayPort = graphicsInfo == null ? null :
graphicsInfo.getPort();
Line 506: model.getDisplay().setPort(displayPort == null ||
displayPort.equals(-1) ? null : displayPort);
Line 507: Integer displaySecurePort = graphicsInfo == null ? null :
graphicsInfo.getTlsPort();
Line 790: }
Line 791:
Line 792: // for backwards compatibility
Line 793: // if vm has multiple framebuffers, returns SPICE
Line 794: @Mapping(from =
org.ovirt.engine.core.common.businessentities.DisplayType.class, to =
String.class)
If this method used outside of this class? If not please change it to "private"
and remove the "@Mapping" annotation. If you choose to keep this as a @Mapping,
then the "from" and "to" parameters are wrong, fix them.
Line 795: public static GraphicsType deriveGraphicsType(Map<GraphicsType,
GraphicsInfo> graphicsInfos, String incoming) {
Line 796: if (graphicsInfos != null) {
Line 797: if (graphicsInfos.containsKey(GraphicsType.SPICE)) {
Line 798: return GraphicsType.SPICE;
Line 791:
Line 792: // for backwards compatibility
Line 793: // if vm has multiple framebuffers, returns SPICE
Line 794: @Mapping(from =
org.ovirt.engine.core.common.businessentities.DisplayType.class, to =
String.class)
Line 795: public static GraphicsType deriveGraphicsType(Map<GraphicsType,
GraphicsInfo> graphicsInfos, String incoming) {
The "incoming" parameter is never used, remove it.
Line 796: if (graphicsInfos != null) {
Line 797: if (graphicsInfos.containsKey(GraphicsType.SPICE)) {
Line 798: return GraphicsType.SPICE;
Line 799: }
Line 800: if (graphicsInfos.containsKey(GraphicsType.VNC)) {
Line 801: return GraphicsType.VNC;
Line 802: }
Line 803: }
Line 804: return null;
This looks like business logic. Can this be moved to the backend? If you are
keeping it in the RESTAPI, just for backwards compatibility, then the method
should probably return an instance of DisplayType
(org.ovirt.engine.api.model.DisplayType) and not GraphicsType.
Line 805: }
Line 806:
Line 807: @Mapping(from = String.class, to = OriginType.class)
Line 808: public static OriginType map(String type, OriginType incoming) {
--
To view, visit http://gerrit.ovirt.org/30837
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0907c9086f49f909250d9956356a0251954f1427
Gerrit-PatchSet: 32
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
