Yair Zaslavsky has posted comments on this change. Change subject: aaa: Changes to ExtensionsManager ......................................................................
Patch Set 19: (10 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. ok, i will remove. 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? : no problem, i'll remove :) 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 i will remove them as you suggested. 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? Done 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? Done 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? what are you refering to exactly? which function? Line 158 Line 159 Line 160 Line 161 Line 162 > maybe put this at static context now? You mean the binding loaders? change the bindingLoaders to static map? 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? Done 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 You mean only setting the TRACE_LOG, right? why do you want to set it in the load, and not in the activate? what benefit can i have from trace log if the extension is not activate? 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 po i agree about the global context. can you please elaborate what do u mean by adding a status field? where exactly. Done about the global context. Line 322: ); Line 323: } Line 324: entry.activated = true; Line 325: dumpConfig(entry.extension); -- 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 <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
