Yair Zaslavsky has posted comments on this change. Change subject: core: Adds custom properties to VmDevice ......................................................................
Patch Set 4: (3 inline comments) .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java Line 593: PERMISSION_REMOVE_FAILED_ONLY_SYSTEM_SUPER_USER_CAN_REMOVE_ADMIN_ROLES, Line 594: CANNOT_ADD_ACTION_GROUPS_TO_ROLE_TYPE, Line 595: ROLE_TYPE_CANNOT_BE_EMPTY, Line 596: ROLE_WITH_ROLE_ID_DO_NOT_EXIST, Line 597: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_SYNTAX, Changes to AppErrors are still missing, + text should be changed (I checked the original texts ) - For more info see - http://www.ovirt.org/Engine_Adding_Messages Line 598: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_KEYS, Line 599: ACTION_TYPE_FAILED_INVALID_CUSTOM_PROPERTIES_INVALID_VALUES, Line 600: CUSTOM_PROPERTIES_INVALID_VALUES_NOT_ALLOWED_IN_CURRENT_CLUSTER, Line 601: .................................................... File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/customprop/DevicePropertiesUtils.java Line 26: Line 27: private static DevicePropertiesUtils devicePropertiesUtils = null; Line 28: Line 29: public static DevicePropertiesUtils getInstance() { Line 30: if (devicePropertiesUtils == null) { I would prefer static initialization, and not using this double check. You can getInstance() for the first time at InitBackendServiceOnStartup bean. Line 31: synchronized (DevicePropertiesUtils.class) { Line 32: if (devicePropertiesUtils == null) { Line 33: devicePropertiesUtils = new DevicePropertiesUtils(); Line 34: } .................................................... File backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/DeviceCustomPropertiesValueHelper.java Line 17: String errMsg = null; Line 18: Line 19: // HACK, because ConfigValues are not loaded inside engine-config Line 20: Version version = new Version(key.getVersion()); Line 21: if (Version.v3_3.compareTo(version) > 0) { I think that wrapping this with if (deviceCustomPropertiesSupported(version)){ ... } as moti suggested, would be nicer. In addtion, shouldn't it be if (version.compareTo(Version.v3_3) >= 0? as the tested version should be 3.3 or above. Line 22: result = false; Line 23: errMsg = String.format("Device custom properties are not supported in version %s", version.toString()); Line 24: } Line 25: -- To view, visit http://gerrit.ovirt.org/14814 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07daa5cccbf8e3cbd6b4191bbc8ebf0729d9d9a0 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
