Michael Pasternak has posted comments on this change.

Change subject: restapi: Adding support for copy/move disk from disk collection.
......................................................................


Patch Set 11: Code-Review-1

(4 comments)

....................................................
File 
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/TemplateDiskResource.java
Line 39:     @Formatted
Line 40:     @Consumes({ApiMediaType.APPLICATION_XML, 
ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML})
Line 41:     @Actionable
Line 42:     @Path("copy")
Line 43:     public Response copy(Action action);
please remove this method, it got inherited from the CopyableResource
Line 44: 
Line 45:     @POST
Line 46:     @Formatted
Line 47:     @Consumes({ApiMediaType.APPLICATION_XML, 
ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML})


....................................................
File 
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 963:       Content-Type: {value: application/xml|json, required: true}
Line 964:       Correlation-Id: {value: 'any string', required: false}
Line 965:       Filter: {value: true|false, required: false}
Line 966:   name: /ovirt-engine/api/disks/{disk:id}/move|rel=move
Line 967:   description: move a disk to a different storage domain. this action 
applicable only for disks attached to a vm.
so why it exposed in context of root-collection /ovirt-engine/api/disks ?
Line 968:   request:
Line 969:     body:
Line 970:       parameterType: Action
Line 971:       signatures:


Line 977:       Content-Type: {value: application/xml|json, required: true}
Line 978:       Correlation-Id: {value: 'any string', required: false}
Line 979:       Filter: {value: true|false, required: false}
Line 980:   name: /ovirt-engine/api/disks/{disk:id}/copy|rel=copy
Line 981:   description: copy a disk to a different storage domain. this action 
applicable only for disks attached to a template.
so why it exposed in context of root-collection /ovirt-engine/api/disks ?
Line 982:   request:
Line 983:     body:
Line 984:       parameterType: Action
Line 985:       signatures:


....................................................
File 
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDiskResource.java
Line 88:                         storageDomainId,
Line 89:                         ImageOperation.Copy);
Line 90:         return doAction(VdcActionType.MoveOrCopyDisk, params, action);
Line 91:     }
Line 92: 
something doesn't look right to me, you saying that these methods should be 
available in the attached disk only, so why they should be outside of 
BackendTemplateDisksResource/BackendVmDisksResource extending 
TemplateDisksResource, VmDiskResource,

why they got implemented in the generic BackendDiskResource ?
Line 93:     @Override
Line 94:     public Disk get() {
Line 95:         return performGet(VdcQueryType.GetDiskByDiskId, new 
IdQueryParameters(guid));
Line 96:     }


-- 
To view, visit http://gerrit.ovirt.org/13973
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I516341d3e033a94a997ac5320312e75a5ed1576c
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[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

Reply via email to