Juan Hernandez has posted comments on this change. Change subject: restapi: Add setupnetworks api ......................................................................
Patch Set 12: (8 comments) https://gerrit.ovirt.org/#/c/35379/12/backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml File backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml: Line 2636: body: Line 2637: parameterType: Action Line 2638: signatures: Line 2639: - mandatoryArguments: {} Line 2640: optionalArguments: { Use indentation instead of curly brackets. Line 2641: action.network_attachments.network_attachment--COLLECTION: { Line 2642: network_attachment.id: 'xs:string', Line 2643: network_attachment.network.id: 'xs:string', Line 2644: network_attachment.host_nic.name|id: 'xs:string', Line 2639: - mandatoryArguments: {} Line 2640: optionalArguments: { Line 2641: action.network_attachments.network_attachment--COLLECTION: { Line 2642: network_attachment.id: 'xs:string', Line 2643: network_attachment.network.id: 'xs:string', Consider adding the network name as an option here, and making the resource support it. This simplifies things for users, as they don't have to lookup the network name before sending the "setupnetwork" request. Line 2644: network_attachment.host_nic.name|id: 'xs:string', Line 2645: network_attachment.ip_configuration.ipv4s.boot_protocol: 'xs:string', Line 2646: network_attachment.ip_configuration.ipv4s.ipv4--COLLECTION: { Line 2647: ipv4.address: 'xs:string', Line 2646: network_attachment.ip_configuration.ipv4s.ipv4--COLLECTION: { Line 2647: ipv4.address: 'xs:string', Line 2648: ipv4.netmask: 'xs:string', Line 2649: ipv4.gateway: 'xs:string' Line 2650: }, Note that the presence of these two levels of nested collections means that the CLI won't be able to use this new action. Not a big problem, we already have it with the current "setupnetworks". Line 2651: network_attachment.properties.property--COLLECTION: { Line 2652: property.name: 'xs:string', Line 2653: property.value: 'xs:string' Line 2654: }, Line 2679: description: initiate the command to setup networks for the specified host Line 2680: urlparams: {} Line 2681: headers: Line 2682: Content-Type: {value: application/xml|json, required: true} Line 2683: Correlation-Id: {value: 'any string', required: false} Remove this ^ empty list of URL parameters and the headers, they are all added implicitly. Line 2684: Line 2685: - name: /hosts/{host:id}/nics/{nic:id}/networkattachments|rel=get Line 2686: description: get the list of network attachments of the network interface Line 2687: request: https://gerrit.ovirt.org/#/c/35379/12/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostResource.java: Line 272: parameters.getRemovedBonds().add(mapBonds(bonds, bond)); Line 273: } Line 274: } Line 275: Line 276: parameters.setRollbackOnFailure(action.isSetCheckConnectivity() ? action.isCheckConnectivity() : false); The RESTAPI should not decide what are default values for backend operations. If the user doesn't provide the "check_connectivity" parameter then the RESTAPI should call "parameters.setRollbackOnFailure". Line 277: if (action.isSetConnectivityTimeout()) { Line 278: parameters.setConectivityTimeout(action.getConnectivityTimeout()); Line 279: } Line 280: Line 316: if (model.isSetId()) { Line 317: Guid nicId = asGuid(model.getId()); Line 318: bond = hostNicMapper.map(model, bonds.get(nicId)); Line 319: } else { Line 320: Bond tempalte = model.isSetName() ? bonds.get(model.getName()) : null; s/tempalte/template/ Line 321: bond = hostNicMapper.map(model, tempalte); Line 322: } Line 323: Line 324: return bond; https://gerrit.ovirt.org/#/c/35379/12/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 113: } Line 114: } Line 115: Line 116: if (model.getBonding().isSetOptions()) { Line 117: StringBuffer buf = new StringBuffer(); StringBuilder performs better. Line 118: for (Option opt : model.getBonding().getOptions().getOptions()) { Line 119: buf.append(opt.getName() + "=" + opt.getValue() + " "); Line 120: } Line 121: entity.setBondOptions(buf.toString().substring(0, buf.length() - 1)); Line 115: Line 116: if (model.getBonding().isSetOptions()) { Line 117: StringBuffer buf = new StringBuffer(); Line 118: for (Option opt : model.getBonding().getOptions().getOptions()) { Line 119: buf.append(opt.getName() + "=" + opt.getValue() + " "); When using a string builder (or buffer) it is better to avoid using string contatenation as well, otherwise you don't get the performance benefit of the builder, as the Java compiler creates another builder for you. It is better to call multiple times the append method: buf.append(opt.getName()); buf.append("=") buf.append(opt.getValue()); Also, I'd suggest to avoid adding and then removing the extra white space, it is confusing: StringBuilder buf = new StringBuilder(); boolean first = true; for (...) { if (!first) { buf.append(" "); } buf.append(...); ... first = false; } Line 120: } Line 121: entity.setBondOptions(buf.toString().substring(0, buf.length() - 1)); Line 122: } Line 123: } -- To view, visit https://gerrit.ovirt.org/35379 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If3f6004956a6b0f17ef5242eb5021429949f65fe Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
