Michael Pasternak has posted comments on this change.
Change subject: engine: watchdog support [wip]
......................................................................
Patch Set 7: (10 inline comments)
1. i can't see mapping code in api for the enums
2. please add RSDL descriptors
3. please add mapping tets
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 888: <xs:element name="watchdog_actions" type="WatchdogActions"/>
Line 889:
Line 890: <xs:complexType name="WatchdogActions">
Line 891: <xs:sequence>
Line 892: <xs:element name="watchdog_action" type="xs:string"
minOccurs="0" maxOccurs="unbounded">
why not just action? you already in context of watchdog
Line 893: <xs:annotation>
Line 894: <xs:appinfo>
Line 895: <jaxb:property name="WatchdogAction"/>
Line 896: </xs:appinfo>
Line 902: <xs:element name="watchdog_models" type="WatchdogModels"/>
Line 903:
Line 904: <xs:complexType name="WatchdogModels">
Line 905: <xs:sequence>
Line 906: <xs:element name="watchdog_model" type="xs:string"
minOccurs="0" maxOccurs="unbounded">
same, "model" is better as you already in context of the Watchdog
Line 907: <xs:annotation>
Line 908: <xs:appinfo>
Line 909: <jaxb:property name="WatchdogModel"/>
Line 910: </xs:appinfo>
Line 1040: </xs:annotation>
Line 1041: </xs:attribute>
Line 1042: </xs:complexType>
Line 1043:
Line 1044: <xs:complexType name="WatchDogType">
1. please define an element for this type
2. you calling it just a watchdog in every place you are refer to it, please
consider renaming this type to just a "Watchdog"
Line 1045: <xs:sequence>
Line 1046: <xs:element name="model" type="xs:string" minOccurs="1"
maxOccurs="1"/>
Line 1047: <xs:element name="action" type="xs:string" minOccurs="1"
maxOccurs="1"/>
Line 1048: </xs:sequence>
Line 1942: <xs:element name="timezone" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 1943: <xs:element ref="domain" minOccurs="0" maxOccurs="1"/>
Line 1944: <xs:element ref="usb" minOccurs="0" maxOccurs="1"/>
Line 1945: <xs:element name="tunnel_migration" type="xs:boolean"
minOccurs="0" maxOccurs="1"/>
Line 1946: <xs:element name="watchdog" type="WatchDogType"
minOccurs="0" maxOccurs="1"/>
please use ref to the element name rather than using type and assigning name to
the element
Line 1947: <!-- also rel="cdroms/disks/nics" links, see Devices below
-->
Line 1948: </xs:sequence>
Line 1949: </xs:extension>
Line 1950: </xs:complexContent>
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/TemplateMapper.java
Line 130: }
Line 131: if (model.isSetTunnelMigration()) {
Line 132: entity.setTunnelMigration(model.isTunnelMigration());
Line 133: }
Line 134: if(model.getWatchdog() != null) {
please use isSetWatchdog() instead
Line 135: entity.setWatchdogAction(model.getWatchdog().getAction());
Line 136: entity.setWatchdogModel(model.getWatchdog().getModel());
Line 137: }
Line 138: return entity;
Line 132: entity.setTunnelMigration(model.isTunnelMigration());
Line 133: }
Line 134: if(model.getWatchdog() != null) {
Line 135: entity.setWatchdogAction(model.getWatchdog().getAction());
Line 136: entity.setWatchdogModel(model.getWatchdog().getModel());
if they are nulls, don't you want to set some default value? i saw you have
NONE in the enum
Line 137: }
Line 138: return entity;
Line 139: }
Line 140:
Line 239: }
Line 240: if (model.isSetTunnelMigration()) {
Line 241: staticVm.setTunnelMigration(model.isTunnelMigration());
Line 242: }
Line 243: if(model.getWatchdog() != null) {
please use isSetWatchdog() instead
Line 244:
staticVm.setWatchdogAction(model.getWatchdog().getAction());
Line 245: staticVm.setWatchdogModel(model.getWatchdog().getModel());
Line 246: }
Line 247: return staticVm;
Line 241: staticVm.setTunnelMigration(model.isTunnelMigration());
Line 242: }
Line 243: if(model.getWatchdog() != null) {
Line 244:
staticVm.setWatchdogAction(model.getWatchdog().getAction());
Line 245: staticVm.setWatchdogModel(model.getWatchdog().getModel());
if they are nulls, don't you want to set some default value? i saw you have
NONE in the enum
Line 246: }
Line 247: return staticVm;
Line 248: }
Line 249:
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java
Line 239: }
Line 240: if (vm.isSetTunnelMigration()) {
Line 241: staticVm.setTunnelMigration(vm.isTunnelMigration());
Line 242: }
Line 243: if(vm.isSetWatchdog()) {
1. please use isSetWatchdog() instead
2. you should have two mapping instances in this file, one for create and
another one for update
Line 244: staticVm.setWatchdogAction(vm.getWatchdog().getAction());
Line 245: staticVm.setWatchdogModel(vm.getWatchdog().getModel());
Line 246: }
Line 247: return staticVm;
Line 241: staticVm.setTunnelMigration(vm.isTunnelMigration());
Line 242: }
Line 243: if(vm.isSetWatchdog()) {
Line 244: staticVm.setWatchdogAction(vm.getWatchdog().getAction());
Line 245: staticVm.setWatchdogModel(vm.getWatchdog().getModel());
if they are nulls, don't you want to set some default value? i saw you have
NONE in the enum
Line 246: }
Line 247: return staticVm;
Line 248: }
Line 249:
--
To view, visit http://gerrit.ovirt.org/12419
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe5c36995dafcef6b3ad45a9e0ca8b0471324b70
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Greg Padgett <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Noam Slomianko <[email protected]>
Gerrit-Reviewer: ofri masad <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
