Juan Hernandez has posted comments on this change. Change subject: engine: Change DisplayType semantics ......................................................................
Patch Set 32: (3 comments) 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); > I changed it so now DisplayType is used. No need to make a mapper if it isn't going to be used outside of this context. 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 516: } Line 517: if (entity.getDefaultDisplayType() != null) { Line 518: model.setDisplay(new Display()); Line 519: GraphicsType graphicsType = deriveGraphicsType(entity.getGraphicsInfos(), null); Line 520: model.getDisplay().setType(graphicsType == null ? null : graphicsType.name().toLowerCase()); > I believe i did this wrong. This branch applies for stopped VM and graphics Where possible RESTAPI logic should not depend on the state of the VM, as that puts its closer to business logic. Ideally it should look like this: if (there are display infos available from the backend, regardless of the state of the VM) { render them to the output document } Line 521: } Line 522: } Line 523: if (entity.getLastStopTime() != null) { Line 524: model.setStopTime(DateMapper.map(entity.getLastStopTime(), null)); Line 800: if (graphicsInfos.containsKey(GraphicsType.VNC)) { Line 801: return GraphicsType.VNC; Line 802: } Line 803: } Line 804: return null; > Hmm, I don't think this belongs to backend. Depends on where you put the responsibility of backwards compatibility. I believe that all components should care about backwards compatibility, including the backend. The RESTAPI shouldn't be the only component that carries the burden of preserving backwards compatibility. However, in this particular case, if you prefer to keep this here just do it. 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 <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[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
