Juan Hernandez has posted comments on this change. Change subject: API: Vm Init - new Feature ......................................................................
Patch Set 20: (8 comments) Some minor comments, mostly cosmetic. http://gerrit.ovirt.org/#/c/23021/20//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2013-12-29 14:39:37 +0200 Line 4: Commit: Shahar Havivi <shav...@redhat.com> Line 5: CommitDate: 2014-01-22 14:46:38 +0200 Line 6: Line 7: API: Vm Init - new Feature Please change the subject prefix to "restapi:". Line 8: Line 9: Summary: Line 10: This Feature will allow persistent of Windows Sysprep and Cloud-Init Line 11: data to the Database. By persisting the data Admin can create a template http://gerrit.ovirt.org/#/c/23021/20/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3077: Line 3078: Line 3079: <!-- Host NICs --> Line 3080: <!-- Host NIC is use for configuration of host hypervisor NIC as well as Line 3081: Initialization NIC which represent initialization - via cloud init and windows Sysprep --> I think that you don't need this now. Line 3082: Line 3083: <xs:element name="host_nic" type="HostNIC"/> Line 3084: Line 3085: <xs:element name="host_nics" type="HostNics"/> Line 3102: <xs:element name="mtu" type="xs:int" minOccurs="0"/> Line 3103: <xs:element name="bridged" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 3104: <xs:element name="custom_configuration" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 3105: <xs:element name="override_configuration" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 3106: <xs:element name="on_boot" type="xs:boolean" minOccurs="0"/> Don't need this either. Line 3107: <!-- Also a rel="master" link for bonded nics --> Line 3108: </xs:sequence> Line 3109: </xs:extension> Line 3110: </xs:complexContent> Line 3130: <xs:sequence> Line 3131: <xs:element name="name" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3132: <xs:element ref="ip" minOccurs="0"/> Line 3133: <xs:element name="boot_protocol" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3134: <xs:element name="on_boot" type="xs:boolean" minOccurs="0"/> The default value of maxOccurs is 1, but you are explicitly putting it in some elements and not in others. Can you add it to the elements that don't have it? Line 3135: </xs:sequence> Line 3136: </xs:complexType> Line 3137: Line 3138: <xs:complexType name="GuestNicsConfiguration"> Line 3138: <xs:complexType name="GuestNicsConfiguration"> Line 3139: <xs:sequence> Line 3140: <xs:element name="nics" type="GuestNicConfiguration" minOccurs="0" maxOccurs="unbounded" /> Line 3141: </xs:sequence> Line 3142: </xs:complexType> Please use the same indentation than the rest of the files: 2 spaces. Line 3143: Line 3144: <xs:element name="host_nic_states" type="HostNICStates"/> Line 3145: Line 3146: <xs:complexType name="HostNICStates"> http://gerrit.ovirt.org/#/c/23021/20/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/HostNicMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/HostNicMapper.java: Line 74: entity.setBondOptions(buf.toString().substring(0, buf.length() - 1)); Line 75: } Line 76: } Line 77: if(model.isSetBootProtocol()){ Line 78: NetworkBootProtocol networkBootProtocol = map(BootProtocol.fromValue(model.getBootProtocol()), NetworkBootProtocol.NONE); Why are you changing this? This patch shouldn't touch the HostNic mapper. Line 79: if(networkBootProtocol != null){ Line 80: entity.setBootProtocol(networkBootProtocol); Line 81: } Line 82: } http://gerrit.ovirt.org/#/c/23021/20/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 1079: return entity; Line 1080: } Line 1081: Line 1082: @Mapping(from = VmInitNetwork.class, to = GuestNicConfiguration.class) Line 1083: public static GuestNicConfiguration map(VmInitNetwork model, GuestNicConfiguration template) { The name of the VmInitNework parameter should be "entity", and the name of the GuestNicConfiguration variable should be "entity". Line 1084: GuestNicConfiguration entity = template != null ? template : new GuestNicConfiguration(); Line 1085: Line 1086: entity.setName(model.getName()); Line 1087: entity.setOnBoot(model.getStartOnBoot()); Line 1110: return null; Line 1111: } Line 1112: } Line 1113: return null; Line 1114: } This method is identical to the one in HostNicMapper. Can you create a new BootProtocolMapper class, put that method there and use it from both HostNicMapper and VmMapper? Line 1115: Line 1116: Line 1117: @Mapping(from = CloudInit.class, to = VmInit.class) Line 1118: public static VmInit map(CloudInit model, VmInit template) { -- To view, visit http://gerrit.ovirt.org/23021 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I817de4fd7c7efcc3740583ede7a96bd522015660 Gerrit-PatchSet: 20 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> 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
