Juan Hernandez has posted comments on this change. Change subject: API: Vm Init - new Feature ......................................................................
Patch Set 14: (13 comments) http://gerrit.ovirt.org/#/c/23021/14/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 594: <xs:element ref="os_types" minOccurs="0"/> Line 595: <xs:element ref="disk_formats" minOccurs="0"/> Line 596: <xs:element ref="disk_interfaces" minOccurs="0"/> Line 597: <xs:element ref="vm_affinities" minOccurs="0"/> Line 598: <xs:element ref="custom_properties" minOccurs="0"/> Please reuse this "boot_protocols" element. Line 599: <xs:element ref="boot_protocols" minOccurs="0"/> Line 600: <xs:element ref="error_handling" minOccurs="0"/> Line 601: <xs:element ref="storage_formats" minOccurs="0"/> Line 602: <xs:element ref="creation_states" minOccurs="0"/> Line 2559: <xs:element ref="domain" minOccurs="0" maxOccurs="1"/> Line 2560: <xs:element name="custom_properties" type="CustomProperties" minOccurs="0"/> Line 2561: <xs:element name="payloads" type="Payloads" minOccurs="0"/> Line 2562: <xs:element name="statistics" type="Statistics" minOccurs="0" maxOccurs="1"/> Line 2563: <xs:element name="disks" type="Disks" minOccurs="0" maxOccurs="1"/> Why not put the new properties inside this element instead of defining a new one? Line 2564: <xs:element ref="initialization" minOccurs="0" maxOccurs="1"/> Line 2565: <xs:element name="nics" type="Nics" minOccurs="0" maxOccurs="1"/> Line 2566: <xs:element name="tags" type="Tags" minOccurs="0" maxOccurs="1"/> Line 2567: <xs:element name="snapshots" type="Snapshots" minOccurs="0" maxOccurs="1"/> Line 2572: <xs:element ref="usb" minOccurs="0" maxOccurs="1"/> Line 2573: <xs:element name="tunnel_migration" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 2574: <xs:element ref="virtio_scsi" minOccurs="0" maxOccurs="1"/> Line 2575: <xs:element ref="permissions" minOccurs="0" maxOccurs="1"/> Line 2576: <xs:element ref="vm_init" minOccurs="0" maxOccurs="1"/> We don't use this kind of "vm_" prefixes in general (there are exceptions). This should be just "init", and we already have an "initialization" element. Line 2577: <xs:element ref="vmpool" minOccurs="0" maxOccurs="1"> Line 2578: <xs:annotation> Line 2579: <xs:appinfo> Line 2580: <jaxb:property name="VmPool"/> Line 2710: <xs:enumeration value="NONE"/> Line 2711: <xs:enumeration value="DHCP"/> Line 2712: <xs:enumeration value="STATIC_IP"/> Line 2713: </xs:restriction> Line 2714: </xs:simpleType> We don't use enumerated types in the RESTAPI. Please use "xs:string" instead of this type. The capabilities resource already contains a "boot_protocols" element that you should reuse, and the BootProtocol class (org.ovirt.engine.api.model.BootProtocol) already contains the values that you need. Even if we deprecate the use of CloudInit we can stick to this, that means less changes in the SDKs. Line 2715: <xs:element name="vm_init_network" type="VmInitNetwork"/> Line 2716: Line 2717: <xs:complexType name="VmInitNetwork"> Line 2718: <xs:sequence> Line 2716: Line 2717: <xs:complexType name="VmInitNetwork"> Line 2718: <xs:sequence> Line 2719: <xs:element name="name" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2720: <xs:element name="start_on_boot" type="xs:boolean" minOccurs="0" maxOccurs="1"/> The "NIC" type also contains this, and it is named just "on_boot", can we name it the same here? Can't we use the existing NetworkConfigration, Nics and NIC elements? Line 2721: <xs:element name="boot_protocol" type="NetBootProtocol" minOccurs="0" maxOccurs="1"/> Line 2722: <xs:element name="ip" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2723: <xs:element name="netmask" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2724: <xs:element name="gateway" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2717: <xs:complexType name="VmInitNetwork"> Line 2718: <xs:sequence> Line 2719: <xs:element name="name" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2720: <xs:element name="start_on_boot" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 2721: <xs:element name="boot_protocol" type="NetBootProtocol" minOccurs="0" maxOccurs="1"/> This should be just "xs:string". Line 2722: <xs:element name="ip" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2723: <xs:element name="netmask" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2724: <xs:element name="gateway" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2725: </xs:sequence> Line 2720: <xs:element name="start_on_boot" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 2721: <xs:element name="boot_protocol" type="NetBootProtocol" minOccurs="0" maxOccurs="1"/> Line 2722: <xs:element name="ip" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2723: <xs:element name="netmask" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2724: <xs:element name="gateway" type="xs:string" minOccurs="0" maxOccurs="1"/> We already have an "IP" type that contains these properties and also the "version" property (to distinguish IPv4 and IPv6. Can we reuse it? Line 2725: </xs:sequence> Line 2726: </xs:complexType> Line 2727: Line 2728: Line 2731: <xs:complexType name="VmInit"> Line 2732: <xs:sequence> Line 2733: <xs:element name="hostname" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2734: <xs:element name="domain" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2735: <xs:element name="timeZone" type="xs:string" minOccurs="0" maxOccurs="1"/> Don't use camel case, use "timezone". Line 2736: <xs:element name="authorized_keys" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2737: <xs:element name="regenerate_keys" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 2738: <xs:element name="dns_servers" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 2739: <xs:element name="dns_search" type="xs:string" minOccurs="0" maxOccurs="1"/> http://gerrit.ovirt.org/#/c/23021/14/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/TemplateMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/TemplateMapper.java: Line 137 Line 138 Line 139 Line 140 Line 141 Doens't this break backwards compatibility? What will happen after the change if the user provides a value for the domain? Line 252 Line 253 Line 254 Line 255 Line 256 Same here, what about backwards compatibility? Line 332 Line 333 Line 334 Line 335 Line 336 Same here, what about backwards compatibility? http://gerrit.ovirt.org/#/c/23021/14/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 1050 Line 1051 Line 1052 Line 1053 Line 1054 What happens with CloudInit files? Are we removing support for them? Line 33: import org.ovirt.engine.api.model.IP; Line 34: import org.ovirt.engine.api.model.IPs; Line 35: import org.ovirt.engine.api.model.MemoryPolicy; Line 36: import org.ovirt.engine.api.model.NIC; Line 37: import org.ovirt.engine.api.model.NetBootProtocol; Use the existing BootProtocol. Line 38: import org.ovirt.engine.api.model.OperatingSystem; Line 39: import org.ovirt.engine.api.model.OsType; Line 40: import org.ovirt.engine.api.model.Payload; Line 41: import org.ovirt.engine.api.model.Quota; -- 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: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[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
