Michael Pasternak has posted comments on this change.
Change subject: engine: watchdog - restapi support
......................................................................
Patch Set 32: I would prefer that you didn't submit this
(22 inline comments)
Lazlo,
please add tests to all *Watchdog resources you've added.
thanks.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddWatchdogCommand.java
Line 29: watchdogDevice.setAddress(StringUtils.EMPTY);
Line 30: watchdogDevice.setSpecParams(getSpecParams());
Line 31: getVmDeviceDao().save(watchdogDevice);
Line 32: setSucceeded(true);
Line 33: setActionReturnValue(watchdogDevice);
should be watchdog id and not entire watchdog
Line 34: }
Line 35:
Line 36: @Override
Line 37: protected boolean canDoAction() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateWatchdogCommand.java
Line 18: getWatchdogs();
Line 19: VmDevice watchdogDevice = watchdogs.get(0); // there must be
only one
Line 20: watchdogDevice.setSpecParams(getSpecParams());
Line 21: getVmDeviceDao().update(watchdogDevice);
Line 22: setActionReturnValue(watchdogDevice);
setSucceeded(true) is enough in update command.
Line 23: setSucceeded(true);
Line 24: }
Line 25:
Line 26: protected boolean canDoAction() {
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 1981: <xs:element name="timezone" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 1982: <xs:element ref="domain" minOccurs="0" maxOccurs="1"/>
Line 1983: <xs:element ref="usb" minOccurs="0" maxOccurs="1"/>
Line 1984: <xs:element name="tunnel_migration" type="xs:boolean"
minOccurs="0" maxOccurs="1"/>
Line 1985: <!-- also rel="cdroms/disks/nics/watchdogs" links, see
Devices below -->
where is a actual link to the watchdogs?
Line 1986: </xs:sequence>
Line 1987: </xs:extension>
Line 1988: </xs:complexContent>
Line 1989: </xs:complexType>
Line 2203: <!-- also rel="snapshots" links, see Snapshots below -->
Line 2204: <!-- also rel="users" links, see Users above -->
Line 2205:
Line 2206: <xs:element ref="reported_devices" minOccurs="0"
maxOccurs="1"/>
Line 2207: <xs:element name="watchdogs" minOccurs="0" maxOccurs="1"/>
should be <xs:element ref="watchdogs" minOccurs="0" maxOccurs="1"/>
Line 2208: </xs:sequence>
Line 2209: </xs:extension>
Line 2210: </xs:complexContent>
Line 2211: </xs:complexType>
Line 2415: </xs:sequence>
Line 2416: </xs:extension>
Line 2417: </xs:complexContent>
Line 2418: </xs:complexType>
Line 2419:
WS
Line 2420: <xs:element name="watchdog" type="WatchDog"/>
Line 2421:
Line 2422: <xs:element name="watchdogs" type="WatchDogs"/>
Line 2423:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendCapabilitiesResource.java
Line 215: addReportedDeviceTypes(version, ReportedDeviceType.values());
Line 216: addIpVersions(version, IpVersion.values());
Line 217: addCpuModes(version, CpuMode.values());
Line 218: addWatchdogActions(version, WatchdogAction.values());
Line 219: addWatchdogModels(version, WatchdogModel.values());
please also add a Feature description for the watchdog.
Line 220:
Line 221: version.setFeatures(featuresHelper.getFeatures(v));
Line 222:
Line 223: if (current == null && VersionHelper.equals(v,
getCurrentVersion())) {
Line 241: }
Line 242: }
Line 243:
Line 244: private void addWatchdogModels(VersionCaps version,
WatchdogModel[] values) {
Line 245: if (VersionUtils.greaterOrEqual(version, VERSION_3_2)) {
this is 3.3 feature, not 3.2
Line 246: version.setWatchdogModels(new WatchdogModels());
Line 247: for (WatchdogModel watchdogModel : values) {
Line 248:
version.getWatchdogModels().getWatchdogModels().add(watchdogModel.value());
Line 249: }
Line 250: }
Line 251: }
Line 252:
Line 253: private void addWatchdogActions(VersionCaps version,
WatchdogAction[] values) {
Line 254: if (VersionUtils.greaterOrEqual(version, VERSION_3_2)) {
this is 3.3 feature, not 3.2
Line 255: version.setWatchdogActions(new WatchdogActions());
Line 256: for (WatchdogAction watchdogAction : values) {
Line 257:
version.getWatchdogActions().getWatchdogActions().add(watchdogAction.value());
Line 258: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendTemplateResource.java
Line 133: }
Line 134:
Line 135: @Path("watchdogs")
Line 136: @Override
Line 137: public DevicesResource<WatchDog, WatchDogs>
getWatchdogsResource() {
you have added dedicated interface "WatchdogsResource" with method
extensions/overrides, - please use it here (in signature)
Line 138: return inject(new BackendWatchdogsResource(guid,
Line 139: VdcQueryType.GetWatchdog,
Line 140: new IdQueryParameters(guid)));
Line 141: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmResource.java
Line 140: new
IdQueryParameters(guid)));
Line 141: }
Line 142:
Line 143: @Override
Line 144: public DevicesResource<WatchDog, WatchDogs>
getWatchdogsResource() {
you have added dedicated interface "WatchdogsResource" with method
overrides/extensions, please use it here (in signature)
Line 145: return inject(new BackendWatchdogsResource(guid,
Line 146:
VdcQueryType.GetWatchdog,
Line 147: new
IdQueryParameters(guid)));
Line 148: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendWatchdogsResource.java
Line 83:
getEntity(org.ovirt.engine.core.common.businessentities.VM.class,
VdcQueryType.GetVmByVmId, queryParams, id.toString());
Line 84: return true;
Line 85: } catch (WebFaultException e) {
Line 86: return false;
Line 87: }
what if WebFaultException caused by absence of permissions to execute this
query?..., i'd suggest using runQuery() and analysing return-value instead.
Line 88: }
Line 89:
Line 90: @Override
Line 91: protected ParametersProvider<WatchDog, VmWatchdog>
getUpdateParametersProvider() {
Line 105: protected WatchDog doPopulate(WatchDog model, VmWatchdog entity) {
Line 106: model.setAction(entity.getAction() == null ? null :
entity.getAction().name());
Line 107: model.setModel(entity.getModel() == null ? null :
entity.getModel().name());
Line 108: model.setId(entity.getId().toString());
Line 109: return model;
doPopulate() is used to execute additional queries to complete the entity, while
what you did should reside in the WatchDogMapper.
Line 110: }
Line 111:
Line 112: @Override
Line 113: public Response add(final WatchDog device) {
Line 110: }
Line 111:
Line 112: @Override
Line 113: public Response add(final WatchDog device) {
Line 114: validateEnums(WatchDog.class, device);
any specific reason for add() overriding?, now you have to call manually all
methods being called from the AbstractBackendDevicesResource.add() such as
validateParameters(device, getRequiredAddFields());
Line 115: return performCreate(addAction,
Line 116: getAddParameters(map(device), device),
Line 117: new IResolver<VmDevice, VmWatchdog>() {
Line 118: public VmWatchdog resolve(VmDevice id) throws
BackendFailureException {
Line 114: validateEnums(WatchDog.class, device);
Line 115: return performCreate(addAction,
Line 116: getAddParameters(map(device), device),
Line 117: new IResolver<VmDevice, VmWatchdog>() {
Line 118: public VmWatchdog resolve(VmDevice id) throws
BackendFailureException {
instead of running query to fetch the created watchdog, you hacking the
resolver (which is used to run the query) to fake created object,
(how do you know that after create() all fields that you asked to appear in the
watchdog actually been set ..., by doing ^, you telling to user something that
not
necessary true)
=> please follow pattern used in other sub-collections.
Line 119: VmWatchdog vmWatchdog = new VmWatchdog();
Line 120:
vmWatchdog.setAction(VmWatchdogAction.getByName(device.getAction()));
Line 121:
vmWatchdog.setModel(VmWatchdogType.getByName(device.getModel()));
Line 122: vmWatchdog.setId(new Guid(device.getId()));
Line 118: public VmWatchdog resolve(VmDevice id) throws
BackendFailureException {
Line 119: VmWatchdog vmWatchdog = new VmWatchdog();
Line 120:
vmWatchdog.setAction(VmWatchdogAction.getByName(device.getAction()));
Line 121:
vmWatchdog.setModel(VmWatchdogType.getByName(device.getModel()));
Line 122: vmWatchdog.setId(new Guid(device.getId()));
this is mapping logic and should be in the mapper only, it is triggered by map()
Line 123: return vmWatchdog;
Line 124: }
Line 125: });
Line 126: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/validation/WatchdogValidator.java
Line 8: @ValidatedClass(clazz = WatchDog.class)
Line 9: public class WatchdogValidator implements Validator<WatchDog> {
Line 10:
Line 11: @Override
Line 12: public void validateEnums(WatchDog entity) {
potential NPE, entity can be NULL
Line 13: EnumValidator.validateEnum(WatchdogAction.class,
entity.getAction(), true);
Line 14: EnumValidator.validateEnum(WatchdogModel.class,
entity.getModel(), false);
Line 15: }
Line 16:
Line 10:
Line 11: @Override
Line 12: public void validateEnums(WatchDog entity) {
Line 13: EnumValidator.validateEnum(WatchdogAction.class,
entity.getAction(), true);
Line 14: EnumValidator.validateEnum(WatchdogModel.class,
entity.getModel(), false);
why you don't want WatchdogModel to be validated in uppercase? all enums are
upper.
Line 15: }
Line 16:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/validation/WatchdogValidatorTest.java
Line 25: @Test()
Line 26: public void validateEnums() {
Line 27: WatchDog entity = new WatchDog();
Line 28: entity.setAction("reset");
Line 29: entity.setModel("i6300esb");
please use enum constants
Line 30: new WatchdogValidator().validateEnums(entity);
Line 31: }
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java
Line 964: VmWatchdog entity = template ==null ? new VmWatchdog() :
template;
Line 965: entity.setAction(model.getAction());
Line 966: entity.setModel(model.getModel());
Line 967: return entity;
Line 968: }*/
these two should move to own mapper
Line 969:
Line 970: static String cpuTuneToString(final CpuTune tune) {
Line 971: final StringBuilder builder = new StringBuilder();
Line 972: boolean first = true;
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/WatchdogModel.java
Line 1: package org.ovirt.engine.api.restapi.types;
Line 2:
Line 3: public enum WatchdogModel {
Line 4: i6300esb,
Line 5: ib700;
should be a UPPER_CASE (value() returns it in lower for representation)
Line 6: public String value () {
Line 7: return this.name().toLowerCase();
Line 8: }
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/WatchdogMapperTest.java
Line 18: entity.setId(new Guid());
Line 19: WatchDog model = WatchdogMapper.map(entity, null);
Line 20: Assert.assertNotNull(model);
Line 21: Assert.assertEquals(model.getAction(), "reset");
Line 22: Assert.assertEquals(model.getModel(), "i6300esb");
any reason for not using constants from the enum? if you'll ever change the
enum,
this tests will produce useless noise.
Line 23: }
Line 24:
Line 25: @Test
Line 26: public void mapWatchdog() {
Line 25: @Test
Line 26: public void mapWatchdog() {
Line 27: WatchDog model = new WatchDog();
Line 28: model.setAction("reset");
Line 29: model.setModel("i6300esb");
same here
Line 30: model.setId(new Guid().toString());
Line 31: VmWatchdog entity = WatchdogMapper.map(model, null);
Line 32: Assert.assertNotNull(entity);
Line 33: Assert.assertEquals(entity.getAction(),
VmWatchdogAction.RESET);
--
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: 32
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
