Michael Pasternak has posted comments on this change.
Change subject: restapi: cloud-init [6/6] - rest api for start vm
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(17 inline comments)
please also add tests for the mapper and vm.start() for this feature.
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 900: </xs:element>
Line 901: </xs:sequence>
Line 902: </xs:complexType>
Line 903:
Line 904: <xs:element name="attachment_types" type="AttachmentTypes"/>
this name is not clear, i'd call it a payload_encodings or similar.
Line 905:
Line 906: <xs:complexType name="AttachmentTypes">
Line 907: <xs:sequence>
Line 908: <xs:element name="attachment_type" type="xs:string"
minOccurs="0" maxOccurs="unbounded">
Line 2086: </xs:annotation>
Line 2087: </xs:element>
Line 2088: </xs:sequence>
Line 2089: </xs:complexType>
Line 2090:
please add type description, e.g new element:
<xs:element name="cloud_init" type="CloudInit"/>
Line 2091: <xs:complexType name="CloudInit">
Line 2092: <xs:sequence>
Line 2093: <xs:element name="hostname" type="xs:string" minOccurs="0"/>
Line 2094: <xs:element name="network" minOccurs="0">
Line 2089: </xs:complexType>
Line 2090:
Line 2091: <xs:complexType name="CloudInit">
Line 2092: <xs:sequence>
Line 2093: <xs:element name="hostname" type="xs:string" minOccurs="0"/>
please use 'host_name'
Line 2094: <xs:element name="network" minOccurs="0">
Line 2095: <xs:complexType>
Line 2096: <xs:sequence>
Line 2097: <xs:element name="interfaces" minOccurs="0">
Line 2093: <xs:element name="hostname" type="xs:string" minOccurs="0"/>
Line 2094: <xs:element name="network" minOccurs="0">
Line 2095: <xs:complexType>
Line 2096: <xs:sequence>
Line 2097: <xs:element name="interfaces" minOccurs="0">
in sake of consistency please reuse "nics" (by adding ref="nics") collection
here instead of defining new types/elements, it contains almost all relevant
properties:
nics[1].nic.network.ip.address|netmask|gateway,
i know generally 'nic.network.ip' is a bit awkward for standard NIC config, but
i'd keep it as is cause this way you create clear isolation between network
configuration & NIC specific properties such as boot_protocol, onboot, etc. +
it will be consistent with cross api modelling of NIC devices.
[1] 1:N
Line 2098: <xs:complexType>
Line 2099: <xs:sequence>
Line 2100: <xs:element name="interface" minOccurs="0"
maxOccurs="unbounded">
Line 2101: <xs:complexType>
Line 2111: </xs:element>
Line 2112: </xs:sequence>
Line 2113: </xs:complexType>
Line 2114: </xs:element>
Line 2115: <xs:element name="dns_servers" minOccurs="0">
please combine all dns related properties in to single type, and refer it from
here, e.g:
<xs:elementref="dns" minOccurs="0">
<dns>
<servers>
<server><server/>
...
<servers/>
<search_domains>
<domain><domain/>
...
<search_domains/>
<dns/>
Line 2116: <xs:complexType>
Line 2117: <xs:sequence>
Line 2118: <xs:element name="dns_server" type="xs:string"
minOccurs="0" maxOccurs="unbounded"/>
Line 2119: </xs:sequence>
Line 2128: </xs:element>
Line 2129: </xs:sequence>
Line 2130: </xs:complexType>
Line 2131: </xs:element> <!-- </network> -->
Line 2132: <xs:element name="authorized_keys" minOccurs="0">
please reuse collection/resource pattern we follow in the schema (see "vm",
"vms" for more details and then just add ref="dns_servers")
<xs:complexType name="VM">
...
<xs:complexType name="VMs">
...
Line 2133: <xs:complexType>
Line 2134: <xs:sequence>
Line 2135: <xs:element name="authorized_key" minOccurs="0"
maxOccurs="unbounded">
Line 2136: <xs:complexType>
Line 2135: <xs:element name="authorized_key" minOccurs="0"
maxOccurs="unbounded">
Line 2136: <xs:complexType>
Line 2137: <xs:simpleContent>
Line 2138: <xs:extension base="xs:string">
Line 2139: <xs:attribute name="user" type="xs:string"
use="required"/>
please reuse "user" type we have in schema
Line 2140: </xs:extension>
Line 2141: </xs:simpleContent>
Line 2142: </xs:complexType>
Line 2143: </xs:element>
Line 2145: </xs:complexType>
Line 2146: </xs:element>
Line 2147: <xs:element name="regenerate_ssh_keys" type="xs:boolean"
minOccurs="0"/>
Line 2148: <xs:element name="timezone" type="xs:string" minOccurs="0"/>
Line 2149: <xs:element name="passwords" minOccurs="0">
please reuse collection/resource pattern we follow in the schema (see "vm",
"vms" for more details and then just add ref="dns_servers")
<xs:complexType name="VM">
...
<xs:complexType name="VMs">
...
Line 2150: <xs:complexType>
Line 2151: <xs:sequence>
Line 2152: <xs:element name="password" minOccurs="0"
maxOccurs="unbounded">
Line 2153: <xs:complexType>
Line 2152: <xs:element name="password" minOccurs="0"
maxOccurs="unbounded">
Line 2153: <xs:complexType>
Line 2154: <xs:simpleContent>
Line 2155: <xs:extension base="xs:string">
Line 2156: <xs:attribute name="user" type="xs:string"
use="required"/>
please reuse "user" type we have in schema
Line 2157: </xs:extension>
Line 2158: </xs:simpleContent>
Line 2159: </xs:complexType>
Line 2160: </xs:element>
Line 2160: </xs:element>
Line 2161: </xs:sequence>
Line 2162: </xs:complexType>
Line 2163: </xs:element>
Line 2164: <xs:element name="files" minOccurs="0">
please reuse "payload_file" [1] type we have in schema (add there path,
encoding props.)
[1] a new collection of payload_file should be added
Line 2165: <xs:complexType>
Line 2166: <xs:sequence>
Line 2167: <xs:element name="file" minOccurs="0"
maxOccurs="unbounded">
Line 2168: <xs:complexType>
Line 2189: <xs:element name="initialization" type="Initialization"/>
Line 2190:
Line 2191: <xs:complexType name="Initialization">
Line 2192: <xs:choice>
Line 2193: <xs:element name="cloud-init" type="CloudInit" minOccurs="0"/>
please refer resource like this ref="cloud_init"
btw we don't use '-' (in properties), but '_'
Line 2194: </xs:choice>
Line 2195: </xs:complexType>
Line 2196:
Line 2197: <xs:complexType name="VmPlacementPolicy">
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 223: cloud-init.passwords.password: 'xs:string',
Line 224: cloud-init.passwords.password.user: 'xs:string',
Line 225: cloud-init.files.file.path: 'xs:string',
Line 226: cloud-init.files.file.content: 'xs:string',
Line 227: cloud-init.files.file.content.encoding: 'xs:string'}
1. note: these will change after schema changes
2. wouldn't you prefer to have 'initialization' in the 'create' rather than
add()?,
it happens only one and i n my view 'initialization' is a part of creation of
vm no?
Line 228: urlparams: {}
Line 229: headers:
Line 230: Content-Type: {value: application/xml|json, required: true}
Line 231: Correlation-Id: {value: 'any string', required: false}
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendCapabilitiesResource.java
Line 234: version.setAttachmentTypes(new AttachmentTypes());
Line 235: for (AttachmentType mode : values) {
Line 236:
version.getAttachmentTypes().getAttachmentTypes().add(mode.value());
Line 237: }
Line 238: }
please also add 'feature' (cloud-init) in this resource (look for other
features for more details).
Line 239: }
Line 240:
Line 241: private void addCpuModes(VersionCaps version, CpuMode[] values) {
Line 242: if (VersionUtils.greaterOrEqual(version, VERSION_3_2)) {
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmResource.java
Line 243
Line 244
Line 245
Line 246
Line 247
any reason for removing vm.isFirstRun()?
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java
Line 967: return entity;
Line 968: }
Line 969:
Line 970: @Mapping(from = Attachment.AttachmentType.class, to =
org.ovirt.engine.api.model.AttachmentType.class)
Line 971: public static org.ovirt.engine.api.model.AttachmentType
map(Attachment.AttachmentType attachmentType,
org.ovirt.engine.api.model.AttachmentType template) {
can't see AttachmentType? are you missing a file?
Line 972: switch (attachmentType) {
Line 973: case BASE64: return
org.ovirt.engine.api.model.AttachmentType.BASE64;
Line 974: case PLAINTEXT: return
org.ovirt.engine.api.model.AttachmentType.PLAINTEXT;
Line 975: default: return null;
Line 1038: }
Line 1039: }
Line 1040: }
Line 1041:
Line 1042: if (model.getTimezone() != null) {
please always use isSetX() for model reference types
Line 1043:
entity.setTimeZone(TimeZone.getTimeZone(model.getTimezone()));
Line 1044: }
Line 1045:
Line 1046: if (model.getPasswords() != null &&
!model.getPasswords().getPassword().isEmpty()) {
Line 1046: if (model.getPasswords() != null &&
!model.getPasswords().getPassword().isEmpty()) {
Line 1047: for (Password password :
model.getPasswords().getPassword()) {
Line 1048: if ("root".equals(password.getUser())) {
Line 1049: entity.setRootPassword(password.getValue());
Line 1050: }
what about else{} ?
Line 1051: }
Line 1052: }
Line 1053:
Line 1054: if (model.getFiles() != null &&
!model.getFiles().getFile().isEmpty()) {
--
To view, visit http://gerrit.ovirt.org/15537
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ad0bfeca23cf8d4b2887010081d63c258032611
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadg...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
