Michael Pasternak has posted comments on this change.
Change subject: engine: watchdog - restapi support
......................................................................
Patch Set 34: I would prefer that you didn't submit this
(11 inline comments)
please also add BackendWatchdogsResourceTest.java (for collection)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 352: request:
Line 353: body:
Line 354: parameterType: CdRom
Line 355: signatures:
Line 356: - mandatoryArguments: {watchdog.action: 'xs:string',
watchdog.name: 'xs:string'}
1. what "watchdog.name" is?, in your code i see that you using: -> "action",
"model"
2. are you sure both are mandatory? i think at least updating "action" only
makes sense, no?,
Line 357: optionalArguments: {watchdog.id: 'xs:string'}
Line 358: urlparams:
Line 359: async: {context: matrix, type: 'xs:boolean', value: true|false,
required: false}
Line 360: current: {context: matrix, type: 'xs:boolean', value:
true|false, required: false}
Line 353: body:
Line 354: parameterType: CdRom
Line 355: signatures:
Line 356: - mandatoryArguments: {watchdog.action: 'xs:string',
watchdog.name: 'xs:string'}
Line 357: optionalArguments: {watchdog.id: 'xs:string'}
id should not be updatable ...
Line 358: urlparams:
Line 359: async: {context: matrix, type: 'xs:boolean', value: true|false,
required: false}
Line 360: current: {context: matrix, type: 'xs:boolean', value:
true|false, required: false}
Line 361: headers:
Line 355: signatures:
Line 356: - mandatoryArguments: {watchdog.action: 'xs:string',
watchdog.name: 'xs:string'}
Line 357: optionalArguments: {watchdog.id: 'xs:string'}
Line 358: urlparams:
Line 359: async: {context: matrix, type: 'xs:boolean', value: true|false,
required: false}
'async' in this context does not make much sense (despite cdrom.update() has it
as well)
Line 360: current: {context: matrix, type: 'xs:boolean', value:
true|false, required: false}
Line 361: headers:
Line 362: Content-Type: {value: application/xml|json, required: true}
Line 363: Correlation-Id: {value: 'any string', required: false}
Line 356: - mandatoryArguments: {watchdog.action: 'xs:string',
watchdog.name: 'xs:string'}
Line 357: optionalArguments: {watchdog.id: 'xs:string'}
Line 358: urlparams:
Line 359: async: {context: matrix, type: 'xs:boolean', value: true|false,
required: false}
Line 360: current: {context: matrix, type: 'xs:boolean', value:
true|false, required: false}
"current" is a cdrom resource (unique) paramater
Line 361: headers:
Line 362: Content-Type: {value: application/xml|json, required: true}
Line 363: Correlation-Id: {value: 'any string', required: false}
Line 364: - name: /api/vms/{vm:id}/watchdogs|rel=add
Line 365: request:
Line 366: body:
Line 367: parameterType: WatchDog
Line 368: signatures:
Line 369: - mandatoryArguments: {watchdog.action: 'xs:string'}
what about watchdog.model? in your implementation i see:
protected String[] getRequiredAddFields() {
return new String[] { "action", "model" };
}
Line 370: urlparams: {}
Line 371: headers:
Line 372: Content-Type: {value: application/xml|json, required: true}
Line 373: Expect: {value: 201-created, required: false}
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendWatchdogsResource.java
Line 21:
Line 22: @Override
Line 23: @SingleEntityResource
Line 24: public DeviceResource<WatchDog> getDeviceSubResource(String id) {
Line 25: return inject(new BackendWatchdogResource(new Guid(), this,
why do you pass 'new Guid()' ?, you should be passing asGuidOr404(id) so
collection item will be accessible,
you was seeing error if you would try to access your watchdog ...
GET .../watchdogs/xxx
Line 26: updateType,
Line 27: getUpdateParametersProvider(),
Line 28: getRequiredUpdateFields()));
Line 29: }
Line 54: }
Line 55:
Line 56: @Override
Line 57: protected String[] getRequiredUpdateFields() {
Line 58: return new String[] { "action", "model" };
are you sure both are mandatory? i think at least updating "action" *only*
makes sense, no?,
Line 59: }
Line 60:
Line 61: @Override
Line 62: protected VdcActionParametersBase getAddParameters(VmWatchdog
entity, WatchDog device) {
Line 63: WatchdogParameters watchdogParameters = new
WatchdogParameters();
Line 64: watchdogParameters.setAction(entity.getAction());
Line 65: watchdogParameters.setModel(entity.getModel());
Line 66: watchdogParameters.setId(parentId);
Line 67: watchdogParameters.setVm(isVm(parentId));
you could figure out that (isVm) in the engine (once you have the id) instead
of forcing all clients to do the same work for you
Line 68: return watchdogParameters;
Line 69: }
Line 70:
Line 71: @Override
Line 71: @Override
Line 72: protected VdcActionParametersBase getRemoveParameters(String id) {
Line 73: WatchdogParameters watchdogParameters = new
WatchdogParameters();
Line 74: watchdogParameters.setId(parentId);
Line 75: watchdogParameters.setVm( isVm(parentId) );
same here
Line 76: return watchdogParameters;
Line 77: }
Line 78:
Line 79: private boolean isVm(Guid id) {
Line 88: WatchdogParameters params = new WatchdogParameters();
Line 89:
params.setModel(VmWatchdogType.getByName(model.getModel()));
Line 90:
params.setAction(VmWatchdogAction.getByName(model.getAction()));
Line 91: params.setId(parentId);
Line 92: params.setVm(isVm(parentId));
same here
Line 93: return params;
Line 94: }};
Line 95: }
Line 96:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendWatchdogResourceTest.java
Line 31: @Override
Line 32: protected VmWatchdog getEntity(int index) {
Line 33: return new VmWatchdog();
Line 34: }
Line 35:
where all the tests? you should be writing same tests as in the
BackendCdRomResourceTest
--
To view, visit http://gerrit.ovirt.org/13060
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief5b20ecf2221faf900cadfeafe4c71607a9eca3
Gerrit-PatchSet: 34
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
