Yair Zaslavsky has posted comments on this change. Change subject: 8. [WIP] core: Introducing the extension interface ......................................................................
Patch Set 1: (4 comments) http://gerrit.ovirt.org/#/c/24811/1/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/Directory.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/Directory.java: Line 10: /** Line 11: * A directory is an object that manages a collection of users and groups, usually stored in an external system like an Line 12: * LDAP database. Line 13: */ Line 14: public abstract class Directory implements Extension, Serializable { > why is it serializable and the authentication is not? 1. we can remove Serializable, now that it's not in common, and not being used by frontend. 2. You mean change "interface Extension" to an abstract class Extension and have Directory and Authenticator extend it? Line 15: /** Line 16: * Line 17: */ Line 18: private static final long serialVersionUID = -8724317446083142917L; http://gerrit.ovirt.org/#/c/24811/1/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/internal/InternalDirectory.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/internal/InternalDirectory.java: Line 44: * Line 45: * @param name the name of the directory Line 46: */ Line 47: public InternalDirectory(String name) { Line 48: setName(name); > why do we need the name here? or maybe it will be removed by future patches There is no particular need, I just modified the code according to the fact it now extends Directory which has a "name" field. Actually, I just realized that I will need to provide default CTORs for all these classes Line 49: // Create the builtin user: Line 50: admin = new DirectoryUser(this.getName(), ADMIN_ID, ADMIN_NAME); Line 51: } Line 52: http://gerrit.ovirt.org/#/c/24811/1/backend/manager/modules/extension-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/Extension.java File backend/manager/modules/extension-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/Extension.java: Line 29: * the properties to be set Line 30: * @throws IllegalArgumentException Line 31: * thrown in case of configuration error Line 32: */ Line 33: void setConfigurationProperties(Properties properties) throws IllegalArgumentException; > setConfiguration... no need for Properties as java is parameters aware... s If you mean the name of the method - no problem, I'll fix it. Line 34: Line 35: /** Line 36: * Gets the configuration properties Line 37: * Line 36: * Gets the configuration properties Line 37: * Line 38: * @return the configuration properties Line 39: */ Line 40: Properties getConfigurationProperties(); > hmmm... maybe we have a problem as java does not know to have two function Why would I want that? I would like to work with Properties for the configuration. Line 41: -- To view, visit http://gerrit.ovirt.org/24811 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic8f583d635c059972a2a536d3c49a58cfcf3234b Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
