Yair Zaslavsky has posted comments on this change. Change subject: aaa: Extensions tester tool ......................................................................
Patch Set 6: (3 comments) http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsTool.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsTool.java: Line 13: import org.ovirt.engine.api.extensions.ExtMap; Line 14: import org.ovirt.engine.api.extensions.ExtUUID; Line 15: import org.ovirt.engine.api.extensions.aaa.Authn; Line 16: import org.ovirt.engine.api.extensions.aaa.Authz; Line 17: import org.ovirt.engine.core.aaa.SearchQueryParsingUtils; > please detach from aaa, single function is not good enough reason. Well, I understand your claim, but if I copy this function to two places, and have a bug, I will need to fix in both cases. I really don't like code duplication (not saying i dont make sins like that sometimes, but if I notice it, i would like to prevent it). Line 18: import org.ovirt.engine.core.extensions.mgr.ExtensionProxy; Line 19: import org.ovirt.engine.core.extensions.mgr.ExtensionsManager; Line 20: Line 21: public class ExtensionsTool { http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolArguments.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolArguments.java: Line 238: parser.addArg(new ArgumentBuilder(). Line 239: longName(ARG_RESOLVE_GROUPS_RECURSIVE). Line 240: valueRequired(false). Line 241: build()); Line 242: > right, just a note so you understand why the above meaningless code is [no I did not like it either. Line 243: } Line 244: return parser; Line 245: } Line 246: http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolExecutor.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolExecutor.java: Line 18: public static void setupLogging(String log4jConfig, String logFile, String logLevel) { Line 19: URL cfgFileUrl = null; Line 20: try { Line 21: if (log4jConfig == null) { Line 22: cfgFileUrl = ExtensionsToolExecutor.class.getResource("/extensions-tool/log4j.xml"); > same issue with class conflict, see the previous patch. already replied there. Line 23: } else { Line 24: cfgFileUrl = new File(log4jConfig).toURI().toURL(); Line 25: } Line 26: Log4jUtils.setupLogging(cfgFileUrl); -- To view, visit http://gerrit.ovirt.org/27814 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7ea2f9c62ced5bdd3801c9f6d8087a35e3c21886 Gerrit-PatchSet: 6 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
