Martin Mucha 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. what's the rule when given style should be used? Should I fix only this curly brace or all following objects containing so many items that it cannot fit on one line? 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 Done 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 I have no idea how this works. Properties is associative array on network_attachment. What's 'property--COLLECTION'? How can it be fixed so it's supported in CLI as well? 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 ad Done 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 operation Done. 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/ Done 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. Done 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 Done 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
