Ori Liel has posted comments on this change.
Change subject: restapi: cloud-init [6/6] - rest api for start vm
......................................................................
Patch Set 14:
(9 comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/PayloadEncoding.java
Line 1: package org.ovirt.engine.api.model;
Line 2:
Line 3: public enum PayloadEncoding {
Line 4: BASE64,
Line 5: PLAINTEXT;
In other enums, when we have two words they are separated by an underscore.
Line 6:
Line 7: public String value() {
Line 8: return name().toLowerCase();
Line 9: }
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 2255: </xs:complexType>
Line 2256:
Line 2257: <xs:element name="dns" type="dns"/>
Line 2258:
Line 2259: <xs:complexType name="dns">
For consistency, first letter should be capital. User either Dns or DNS
Line 2260: <xs:sequence>
Line 2261: <xs:element name="servers">
Line 2262: <xs:complexType>
Line 2263: <xs:sequence>
Line 2260: <xs:sequence>
Line 2261: <xs:element name="servers">
Line 2262: <xs:complexType>
Line 2263: <xs:sequence>
Line 2264: <xs:element name="dns_server" type="xs:string"
minOccurs="0" maxOccurs="unbounded"/>
Either have [servers, server] or [dns_servers, dns_server].
(everywhere else the name of the collection is the same as the name of the
single entity, except for the 's' which is appended at the end).
Line 2265: </xs:sequence>
Line 2266: </xs:complexType>
Line 2267: </xs:element>
Line 2268: <xs:element name="search_domains">
Line 2308: <xs:element ref="authorized_keys" minOccurs="0"/>
Line 2309: <xs:element name="regenerate_ssh_keys" type="xs:boolean"
minOccurs="0"/>
Line 2310: <xs:element name="timezone" type="xs:string" minOccurs="0"/>
Line 2311: <xs:element ref="users" minOccurs="0"/>
Line 2312: <xs:element name="files" minOccurs="0">
For all of the above, since you already explicitly addressed the minOccurs, add
maxOccurs as well.
* Don't be confused by collection elements - files, users, authorized_keys -
they all shoule have maxOccurs=1, because they are the container of the
collection, not the collection itself.
Line 2313: <xs:complexType>
Line 2314: <xs:sequence>
Line 2315: <xs:element name="file" minOccurs="0"
maxOccurs="unbounded">
Line 2316: <xs:complexType>
Line 2319: <xs:element name="content">
Line 2320: <xs:complexType>
Line 2321: <xs:simpleContent>
Line 2322: <xs:extension base="xs:string">
Line 2323: <xs:attribute name="encoding"
type="xs:string" use="required"/>
There is currently only one thing under 'content', which is 'encoding', so why
did you choose a heirarchial structure?
<file>
<content>
<encoding>xxx</encoding>
<content>
</file>
Why not simply:
<file>
<encoding>xxx</encoding>
</file>
or:
<file>
<content_encoding>xxx</content_encoding>
</file>
Is it because you assume there will be more things in the future to put under
<content>?
Line 2324: </xs:extension>
Line 2325: </xs:simpleContent>
Line 2326: </xs:complexType>
Line 2327: </xs:element>
Line 2336:
Line 2337: <xs:element name="initialization" type="Initialization"/>
Line 2338:
Line 2339: <xs:complexType name="Initialization">
Line 2340: <xs:choice>
Why use 'xs:choice'? xs:choice allows only one of the elements contained in the
<choice> declaration to be present within the containing element. It's
basically a way to say "either-or". I think <xs:sequence> is more appropriate
here.
Line 2341: <xs:element name="cloud_init" type="CloudInit" minOccurs="0"/>
Line 2342: </xs:choice>
Line 2343: </xs:complexType>
Line 2344:
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 213: action.async: 'xs:boolean', action.vm.os.kernel:
'xs:string', action.grace_period.expiry: 'xs:long',
Line 214: action.vm.display.type: 'xs:string', action.vm.stateless:
'xs:boolean', action.vm.os.cmdline: 'xs:string',
Line 215: action.vm.domain.user.username: 'xs:string', action.pause:
'xs:boolean',
Line 216: action.vm.os.boot--COLLECTION: {boot.dev: 'xs:string'},
action.vm.domain.user.password: 'xs:string'}
Line 217: action.vm.initialization.cloud-init--COLLECTION:
{cloud-init.hostname: 'xs:string',
There are many things here that appear to me to be wrong, let's talk about it.
Line 218: cloud-init.network.interfaces.name: 'xs:string',
Line 219: cloud-init.network.interfaces.boot_protocol: 'xs:string',
Line 220: cloud-init.network.interfaces.address: 'xs:string',
Line 221: cloud-init.network.interfaces.netmask: 'xs:string',
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java
Line 903: entity.setHostname(model.getHostName());
Line 904: }
Line 905:
Line 906: if (model.isSetAuthorizedKeys() &&
!model.getAuthorizedKeys().getAuthorizedKey().isEmpty()) {
Line 907: StringBuilder keys = new StringBuilder();
I understand that right not only root is supported by engine, but I prefer that
the API layer will pass to the engine all the information that the user
provided. Engine may choose to ignore, for now, all users aside for 'root'.
The way it's written here, is just a bug waiting to happen - tomorrow engine
will support other users, and suddenly we will have a bug in the API: some
users are not passed on to the engine, for some reason.
Line 908: for (AuthorizedKey authKey :
model.getAuthorizedKeys().getAuthorizedKey()) {
Line 909: if ("root".equals(authKey.getUser().getName())) {
Line 910: if (keys.length() > 0) {
Line 911: keys.append("\n");
Line 927: VdsNetworkInterface vdsNetworkInterface = new
VdsNetworkInterface();
Line 928: NetworkBootProtocol protocol =
HostNicMapper.map(BootProtocol.fromValue(iface.getBootProtocol()), null);
Line 929: vdsNetworkInterface.setBootProtocol(protocol);
Line 930: if (protocol != NetworkBootProtocol.DHCP) {
Line 931:
vdsNetworkInterface.setAddress(iface.getNetwork().getIp().getAddress());
I think there's a potential NullPointerException here. What if network is not
set in the nic? or Ip is not set in the network? or Address is not set in Ip?
Line 932:
vdsNetworkInterface.setSubnet(iface.getNetwork().getIp().getNetmask());
Line 933:
vdsNetworkInterface.setGateway(iface.getNetwork().getIp().getGateway());
Line 934: }
Line 935: if (iface.isOnBoot()) {
--
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: 14
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: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Ori Liel <ol...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@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
