Michael Pasternak has posted comments on this change.
Change subject: restapi: Add support for vnic profiles
......................................................................
Patch Set 8: Code-Review-1
(10 comments)
well done Moti!, few comments inline
....................................................
Commit Message
Line 9: The patch adds support for the vnic profiles model.
Line 10: The vnic profiles is added as a top collection and also
Line 11: as a sub collection of the network resource:
Line 12: /api/vnicprofiles/
Line 13: /api/networks/{network:id}/vnicprofiles
please add body to be POST'ed
Line 14:
Line 15: Since the vnic profile is created for a network and it is
Line 16: not supported to switch an existing vnic profile between
Line 17: networks, the update action of the vnic profile is not
Line 15: Since the vnic profile is created for a network and it is
Line 16: not supported to switch an existing vnic profile between
Line 17: networks, the update action of the vnic profile is not
Line 18: supported under the networks collection, only under the
Line 19: top collection of the vnic profile.
- please add ref to wiki / BZ
- please describe this feature in FeatureHelper
Line 20:
Line 21: Change-Id: I7a0d472215ec1b0c359c0e2012e142ae8a627ce9
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 2347: request:
Line 2348: body:
Line 2349: parameterType: null
Line 2350: signatures: []
Line 2351: urlparams: {}
please add [1] to urlparams
[1] max: {context: matrix, type: 'xs:int', value: 'max results', required:
false}
Line 2352: headers: {}
Line 2353: - name: /api/vnicprofiles/{vnicprofile:id}|rel=get
Line 2354: request:
Line 2355: body:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/AbstractBackendVnicProfilesResource.java
Line 23:
Line 24: protected VnicProfiles
mapCollection(List<org.ovirt.engine.core.common.businessentities.network.VnicProfile>
entities) {
Line 25: VnicProfiles collection = new VnicProfiles();
Line 26: for
(org.ovirt.engine.core.common.businessentities.network.VnicProfile entity :
entities) {
Line 27: collection.getVnicProfiles().add(addLinks(map(entity)));
please add populate call, i.e addLinks(populate(map(entity), ...))
Line 28: }
Line 29:
Line 30: return collection;
Line 31: }
Line 30: return collection;
Line 31: }
Line 32:
Line 33: protected Response add(VnicProfile vnicProfile) {
Line 34:
org.ovirt.engine.core.common.businessentities.network.VnicProfile entity =
map(vnicProfile);
1. i would add call here for abstract validate() method, this way you'll make
sure all children of this class will have own validateParameters() impl and it
always get called
2. not sure you need this local var., you could simply pass map() as arg to
performCreate()
Line 35: return performCreate(VdcActionType.AddVnicProfile,
Line 36: new VnicProfileParameters(entity),
Line 37: new
QueryIdResolver<Guid>(VdcQueryType.GetVnicProfileById,
IdQueryParameters.class));
Line 38: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendAssignedVnicProfilesResource.java
Line 36:
Line 37: return super.add(vnicProfile);
Line 38: }
Line 39:
Line 40: @Override
please add @SingleEntityResource
Line 41: public AssignedVnicProfileResource
getAssignedVnicProfileSubResource(@PathParam("id") String id) {
Line 42: return inject(new BackendAssignedVnicProfileResource(id,
this));
Line 43: }
Line 44:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVnicProfileResource.java
Line 20: }
Line 21:
Line 22: @Override
Line 23: public VnicProfile get() {
Line 24: return addLinks(super.get());
no need to call addLinks() explicitly, this is already done by performGet()
called from super.get()
Line 25: }
Line 26:
Line 27: @Override
Line 28: public VnicProfile update(VnicProfile resource) {
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVnicProfilesResource.java
Line 34: validateParameters(vnicProfile, "name", "network.id");
Line 35: return super.add(vnicProfile);
Line 36: }
Line 37:
Line 38: @Override
please add @SingleEntityResource
Line 39: public VnicProfileResource
getVnicProfileSubResource(@PathParam("id") String id) {
Line 40: return inject(new BackendVnicProfileResource(id));
Line 41: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/AbstractBackendVnicProfilesResourceTest.java
Line 203: @Ignore
Line 204: @Override
Line 205: public void testListCrashClientLocale() throws Exception {
Line 206: }
Line 207:
any specific reason for disabling these three tests?
Line 208: protected abstract void setGetAllVnicProfilesExpectations();
Line 209:
Line 210: protected void setUpEntityQueryExpectations(int times, int index,
boolean notFound) throws Exception {
Line 211: while (times-- > 0) {
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVnicProfilesResourceTest.java
Line 35:
Line 36: @Override
Line 37: protected List<VnicProfile> getCollection() {
Line 38: return collection.list().getVnicProfiles();
Line 39: }
- please add test/s for add()
- please add test/s for remove()
--
To view, visit http://gerrit.ovirt.org/17587
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7a0d472215ec1b0c359c0e2012e142ae8a627ce9
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@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
