Alon Bar-Lev has posted comments on this change. Change subject: aaa: Changes to ExtensionsManager ......................................................................
Patch Set 19: (11 comments) http://gerrit.ovirt.org/#/c/27785/19/backend/manager/dependencies/src/main/modules/org/apache/commons/logging/main/modules.xml File backend/manager/dependencies/src/main/modules/org/apache/commons/logging/main/modules.xml: Line 3: <module xmlns="urn:jboss:module:1.1" name="org.apache.commons.logging"> Line 4: Line 5: <resources> Line 6: <resource-root path="commons-logging.jar"/> Line 7: </resources> why do you need this? jboss already had it. $JBOSS_HOME/modules/org/apache/commons/logging/main/module.xml http://gerrit.ovirt.org/#/c/27785/19/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthn.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthn.java: Line 32 Line 33 Line 34 Line 35 Line 36 is it that important to leave this function instead of not call anything? :) http://gerrit.ovirt.org/#/c/27785/19/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java: Line 78 Line 79 Line 80 Line 81 Line 82 if you leave it, please order by call order... init post load http://gerrit.ovirt.org/#/c/27785/19/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthn.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthn.java: Line 83: } Line 84: Line 85: Line 86: Line 87: why do we need these spaces? Line 88: /** Line 89: * {@inheritDoc} Line 90: */ Line 91: private void doAuthenticate(ExtMap input, ExtMap output) { http://gerrit.ovirt.org/#/c/27785/19/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthz.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthz.java: Line 134: try { Line 135: Object command = input.get(Base.InvokeKeys.COMMAND); Line 136: if (command.equals(Base.InvokeCommands.LOAD)) { Line 137: doLoad(input, output); Line 138: } else why new line? Line 139: if (command.equals(Base.InvokeCommands.INITIALIZE)) { Line 140: doInit(input, output); Line 141: } else if (command.equals(Authz.InvokeCommands.QUERY_OPEN)) { Line 142: doQueryOpen(input, output); http://gerrit.ovirt.org/#/c/27785/19/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java File backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java: Line 131 Line 132 Line 133 Line 134 Line 135 why do you need this as function? what's wrong with the way it was? Line 158 Line 159 Line 160 Line 161 Line 162 maybe put this at static context now? Line 262: for (ExtensionEntry entry : loadedEntries.values()) { Line 263: results.add(entry.extension); Line 264: } Line 265: return results; Line 266: } can we have all getXXX at one place? Line 267: Line 268: public ExtensionProxy activate(String extensionName) { Line 269: ExtensionEntry entry = loadedEntries.get(extensionName); Line 270: if (entry == null) { Line 271: throw new RuntimeException(String.format("No extensioned with instance name %1$s could be found", Line 272: extensionName)); Line 273: } Line 274: if (entry.enabled) { Line 275: entry.extension.getContext().put( this should be at load Line 276: TRACE_LOG_CONTEXT_KEY, Line 277: LogFactory.getLog( Line 278: String.format( Line 279: "%1$s.%2$s.%3$s", Line 317: entry.extension.getExtension() Line 318: ).mput( Line 319: Base.ExtensionRecord.CONTEXT, Line 320: entry.extension.getContext() Line 321: ) I think this can be at load... and add a status field. update the status post initialize. Line 322: ); Line 323: } Line 324: entry.activated = true; Line 325: dumpConfig(entry.extension); http://gerrit.ovirt.org/#/c/27785/19/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/extensionsmgr/EngineExtensionsManager.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/extensionsmgr/EngineExtensionsManager.java: Line 67: if (EngineLocalConfig.getInstance().getBoolean(ENGINE_EXTENSION_ENABLED Line 68: + extension.getContext().<String> get(Base.ContextKeys.INSTANCE_NAME), Line 69: true)) { Line 70: try { Line 71: instance.activate(extension.getContext().<String> get(Base.ContextKeys.INSTANCE_NAME)); ok... I do not follow, maybe you have done this... if extension is enabled = false at properties it should not be enabled unless we have explicit configuration at engine config. in other words if there is a value in engine config it override the extension properties... but if there is no, we should take the properties into account. Line 72: } catch (Exception ex) { Line 73: log.error(ex.getMessage()); Line 74: log.debug(ex); Line 75: } -- To view, visit http://gerrit.ovirt.org/27785 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1c914df29a0dbf52ff6d2f8149687b31b4faffe1 Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [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
