Alon Bar-Lev has posted comments on this change. Change subject: aaa: Extensions tester tool ......................................................................
Patch Set 6: (13 comments) quick notes before diving http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/pom.xml File backend/manager/extension-tool/pom.xml: Line 22: <dependency> Line 23: <groupId>${engine.groupId}</groupId> Line 24: <artifactId>common</artifactId> Line 25: <version>${engine.version}</version> Line 26: </dependency> it should not depend on common Line 27: <dependency> Line 28: <groupId>${engine.groupId}</groupId> Line 29: <artifactId>aaa</artifactId> Line 30: <version>${engine.version}</version> Line 27: <dependency> Line 28: <groupId>${engine.groupId}</groupId> Line 29: <artifactId>aaa</artifactId> Line 30: <version>${engine.version}</version> Line 31: </dependency> it should not depend on aaa Line 32: <dependency> Line 33: <groupId>${engine.groupId}</groupId> Line 34: <artifactId>utils</artifactId> Line 35: <version>${engine.version}</version> Line 32: <dependency> Line 33: <groupId>${engine.groupId}</groupId> Line 34: <artifactId>utils</artifactId> Line 35: <version>${engine.version}</version> Line 36: </dependency> I sure like to believe that on utils either Line 37: <dependency> Line 38: <groupId>commons-lang</groupId> Line 39: <artifactId>commons-lang</artifactId> Line 40: </dependency> 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"); qfdn of class please but why cant' we use the standard java logging? Line 23: } else { Line 24: cfgFileUrl = new File(log4jConfig).toURI().toURL(); Line 25: } Line 26: Log4jUtils.setupLogging(cfgFileUrl); Line 55: try { Line 56: if (testerArgs.contains(ACTION_HELP)) { Line 57: testerArgs.printHelp(); Line 58: System.exit(0); Line 59: } else { ? Line 60: } Line 61: } catch (Exception e) { Line 62: } Line 63: } Line 57: testerArgs.printHelp(); Line 58: System.exit(0); Line 59: } else { Line 60: } Line 61: } catch (Exception e) { ? Line 62: } Line 63: } Line 64: http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/resources/extensions-tool/help.properties File backend/manager/extension-tool/src/main/resources/extensions-tool/help.properties: Line 1: help.usage=usage: extensions-tool <action> [<args>] I hate we duplicate usage between code and file... remove this until we close all up Line 2: Line 3: help.actions=\ Line 4: Available actions:\ Line 5: \n\tauth-sequence runs an authentication sequence (authenticate, fetch principal, logout)\ http://gerrit.ovirt.org/#/c/27814/6/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 296: entry.activated = true; Line 297: dumpConfig(entry.extension); Line 298: setChanged(); Line 299: notifyObservers(); Line 300: return entry.extension; squash this into[1]? [1] http://gerrit.ovirt.org/#/c/27785/ Line 301: } Line 302: } Line 303: Line 304: private Extension loadExtension(Properties props) throws Exception { http://gerrit.ovirt.org/#/c/27814/6/backend/manager/pom.xml File backend/manager/pom.xml: Line 15: <module>modules</module> Line 16: <module>tools</module> Line 17: <module>extension-tool</module> Line 18: </modules> Line 19: </project> squash this into[1]? [1] http://gerrit.ovirt.org/#/c/27785/ http://gerrit.ovirt.org/#/c/27814/6/backend/manager/tools/src/main/resources/extensions-tester/jaas.conf File backend/manager/tools/src/main/resources/extensions-tester/jaas.conf: Line 1 Line 2 Line 3 Line 4 squash into previous? http://gerrit.ovirt.org/#/c/27814/6/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 556: # Line 557: # /var creation Line 558: # Line 559: install -dm 755 "%{buildroot}/%{engine_state}"/{content,setup/answers} Line 560: install -dm 755 "%{buildroot}/%{engine_log}"/{host-deploy,setup,notifier,engine-manage-domains,dump,extensions-tool} you should not put logs into /var when running utilities. this is the same discussion as the engine-config or manage domain. see[1] of how to manage logs. [1] http://gerrit.ovirt.org/#/c/26591/ Line 561: install -dm 755 "%{buildroot}/%{engine_cache}" Line 562: install -dm 755 "%{buildroot}/%{engine_run}/notifier" Line 563: Line 564: # Line 1006: %ghost %config(noreplace) %{engine_etc}/notifier/notifier.conf Line 1007: %{_bindir}/engine-backup Line 1008: %{_bindir}/engine-config Line 1009: %{_bindir}/engine-manage-domains Line 1010: %{_bindir}/extensions-tool ovirt-engine prefix for new tools please Line 1011: %{_mandir}/man8/engine-backup.* Line 1012: %{_mandir}/man8/engine-config.* Line 1013: %{_mandir}/man8/engine-manage-domains.* Line 1014: %{engine_data}/bin/engine-backup.sh Line 1014: %{engine_data}/bin/engine-backup.sh Line 1015: %{engine_data}/bin/engine-config.sh Line 1016: %{engine_data}/bin/engine-manage-domains.sh Line 1017: %{engine_data}/bin/engine-prolog.sh Line 1018: %{engine_data}/bin/extensions-tool.sh ovirt-engine prefix for new tools please Line 1019: %{engine_data}/bin/ovirt-engine-role.sh Line 1020: %{engine_data}/conf/jaas.conf Line 1021: %{engine_data}/services/ovirt-engine-notifier Line 1022: %{engine_etc}/engine-config/engine-config.*properties -- 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 <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
