Ori Liel has posted comments on this change. Change subject: restapi: Add REST API support for iSCSI multipathing ......................................................................
Patch Set 14: (7 comments) http://gerrit.ovirt.org/#/c/26225/14/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/IscsiBondsResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/IscsiBondsResource.java: Line 11: Line 12: import org.ovirt.engine.api.model.IscsiBond; Line 13: import org.ovirt.engine.api.model.IscsiBonds; Line 14: Line 15: @Path("/iscsibonds") If I'm not mistaken, this @Path annotation is unnecessary (and perhaps may cause confusion) because this is not a root collection. Iscsi bonds only appear in the context of a datacenter: ..../api/datacenters/xxx/iscsibons And not: .../api/iscsibonds Line 16: @Produces({ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML}) Line 17: public interface IscsiBondsResource { Line 18: Line 19: @GET http://gerrit.ovirt.org/#/c/26225/14/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 5701: parameterType: null Line 5702: signatures: [] Line 5703: urlparams: {} Line 5704: headers: {} Line 5705: >From what I can tell, there are missing signatures: anything that can be done under: /datacenters/{datacenter:id}/iscsibonds/{iscsibond:id}/networks and anything that can be done under: /datacenters/{datacenter:id}/iscsibonds/{iscsibond:id}/storageconnections http://gerrit.ovirt.org/#/c/26225/14/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDataCenterIscsiBondsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDataCenterIscsiBondsResource.java: Line 16: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 17: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 18: import org.ovirt.engine.core.compat.Guid; Line 19: Line 20: public class BackendDataCenterIscsiBondsResource extends AbstractBackendCollectionResource<IscsiBond, org.ovirt.engine.core.common.businessentities.IscsiBond> I feel that the naming is a bit confusing: the interface is IscsiBondsResource and the only implementing class is BackendDataCenterIscsiBondsResource. I would be better to have any of these couples: IscsiBondsResource && BackendIscsiBondsResource OR DataCenterIscsiBondsResource && BackendDataCenterIscsiBondsResource Not a must I guess, but I have to admit it confused me while going over the patch. Line 21: implements IscsiBondsResource { Line 22: Line 23: static final String[] SUB_COLLECTIONS = {"storageconnections", "networks"}; Line 24: Line 37: } Line 38: Line 39: @Override Line 40: public Response add(IscsiBond iscsiBond) { Line 41: org.ovirt.engine.core.common.businessentities.IscsiBond entity = iscsi-bond name and datacenter id are mandatory parameters, so their exsitence should be validated (use validateParameter()) Line 42: getMapper(IscsiBond.class, org.ovirt.engine.core.common.businessentities.IscsiBond.class).map(iscsiBond, null); Line 43: Line 44: entity.setStoragePoolId(dataCenterId); Line 45: http://gerrit.ovirt.org/#/c/26225/14/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendIscsiBondResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendIscsiBondResource.java: Line 12: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 13: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 14: import org.ovirt.engine.core.compat.Guid; Line 15: Line 16: public class BackendIscsiBondResource extends AbstractBackendActionableResource<IscsiBond, org.ovirt.engine.core.common.businessentities.IscsiBond> Again, the names of the classes are confusing: Plural: BackendDataCenterIscsiBondsResource Singular: BackendIscsiBondResource We should either have: Plural: BackendIscsiBondsResource Singular: BackendIscsiBondResource OR Plural: BackendDataCenterIscsiBondsResource Singular: BackendDataCenterIscsiBondResource Line 17: implements IscsiBondResource { Line 18: Line 19: public BackendIscsiBondResource(String id) { Line 20: super(id, IscsiBond.class, org.ovirt.engine.core.common.businessentities.IscsiBond.class, SUB_COLLECTIONS); http://gerrit.ovirt.org/#/c/26225/14/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendDataCenterIscsiBondsResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendDataCenterIscsiBondsResourceTest.java: Line 1: package org.ovirt.engine.api.restapi.resource; Missing a test for remove. There's only a test case for remove non-existant (trying to remove entity which does not exist). The flow of a valid 'remove' is a main flow; should have a test method for it. Line 2: Line 3: import java.util.ArrayList; Line 4: import java.util.List; Line 5: http://gerrit.ovirt.org/#/c/26225/14/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendIscsiBondNetworksResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendIscsiBondNetworksResourceTest.java: Line 72: setUriInfo(setUpBasicUriExpectations()); Line 73: Network network = new Network(); Line 74: network.setId(NETWORK_ID.toString()); Line 75: Line 76: setUpCreationExpectations(VdcActionType.EditIscsiBond, Dont use setUpCreationExpectations, split it into two expectations Line 77: EditIscsiBondParameters.class, Line 78: new String[] { "IscsiBond" }, Line 79: new Object[] { getIscsiBondWithNoNetworks() }, Line 80: true, -- To view, visit http://gerrit.ovirt.org/26225 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id8881110cbeb163e9fc09e98bf4497d894f40490 Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: [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
