Alon Bar-Lev has posted comments on this change. Change subject: aaa: extensions test tool: logger ......................................................................
Patch Set 1: (12 comments) http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/pom.xml File backend/manager/extension-tool/pom.xml: Line 39: <groupId>commons-logging</groupId> Line 40: <artifactId>commons-logging</artifactId> Line 41: <version>${commons-logging.version}</version> Line 42: <scope>compile</scope> Line 43: </dependency> log4j please Line 44: </dependencies> Line 45: Line 46: <build> Line 47: <plugins> http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolArguments.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolArguments.java: Line 6: import java.util.Map; Line 7: import java.util.Properties; Line 8: import java.util.Set; Line 9: Line 10: import com.sun.org.apache.xpath.internal.operations.Mod; what is that? Line 11: import org.ovirt.engine.core.uutils.cli.ArgumentBuilder; Line 12: import org.ovirt.engine.core.uutils.cli.ExtendedCliParser; Line 13: Line 14: public class ExtensionsToolArguments { Line 7: import java.util.Properties; Line 8: import java.util.Set; Line 9: Line 10: import com.sun.org.apache.xpath.internal.operations.Mod; Line 11: import org.ovirt.engine.core.uutils.cli.ArgumentBuilder; oh! you are using this one... I hate this class... but well... maybe we improve it as part of this work. Line 12: import org.ovirt.engine.core.uutils.cli.ExtendedCliParser; Line 13: Line 14: public class ExtensionsToolArguments { Line 15: http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/Module.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/Module.java: Line 1: package org.ovirt.engine.exttool.core; Line 2: Line 3: public enum Module { this class is bad as you do not want to patch it when adding new modules, the entire idea of services is that you can dynamically detect them at runtime. Line 4: Line 5: AAA("aaa"), Line 6: LOGGER("logger"); Line 7: http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java: Line 1: package org.ovirt.engine.exttool.core; Line 2: Line 3: public interface ModuleService { Line 4: Line 5: public void runModule(ExtensionsToolArguments args) throws Exception; I expect at least: 1. get usage 2. parse arguments 3. run probably in future run should be split as well Line 6: http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java: Line 2: Line 3: Line 4: import org.ovirt.engine.exttool.core.ModuleService; Line 5: Line 6: public interface LoggerService extends ModuleService { so why do you need this interface? Line 7: http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/resources/META-INF/services/org.ovirt.engine.exttool.logger.services.LoggerService File backend/manager/extension-tool/src/main/resources/META-INF/services/org.ovirt.engine.exttool.logger.services.LoggerService: Line 1: org.ovirt.engine.exttool.logger.services.LoggerServiceImpl please make sure you put new lines at end of file http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help.properties File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help.properties: Line 8: \n\t\t\t\tLogger::FLUSH\ Line 9: \n\t\t\t\tLogger::CLOSE\ Line 10: \n\t\t\t--logger-name="org.ovirt.engine"\ Line 11: \n\t\t\t--level=INFO\ Line 12: \n\t\t\t--message="test" hmmm... won't it be simpler to just put text file as resource? http://gerrit.ovirt.org/#/c/37886/1/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 655 Line 656 Line 657 Line 658 Line 659 please sort() Line 1127: %{engine_data}/bin/engine-config.sh Line 1128: %{engine_data}/bin/engine-manage-domains.sh Line 1129: %{engine_data}/bin/engine-prolog.sh Line 1130: %{engine_data}/bin/ovirt-engine-role.sh Line 1131: %{engine_data}/bin/ovirt-engine-extensions-tool.sh please sort() Line 1132: %{engine_data}/conf/jaas.conf Line 1133: %{engine_data}/conf/notifier-logging.properties Line 1134: %{engine_data}/conf/tools-logging.properties Line 1135: %{engine_data}/services/ovirt-engine-notifier http://gerrit.ovirt.org/#/c/37886/1/packaging/bin/ovirt-engine-extensions-tool.sh File packaging/bin/ovirt-engine-extensions-tool.sh: Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \ Line 26: -dependencies org.ovirt.engine.core.extension-tool \ Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \ Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \ Line 29: "$@" please copy as much as you can packaging/bin/engine-manage-domains.sh, for example no need for classpath and such http://gerrit.ovirt.org/#/c/37886/1/pom.xml File pom.xml: Line 45: <!-- Dependencies Versions --> Line 46: <junit.version>4.11</junit.version> Line 47: <commons-codec.version>1.4</commons-codec.version> Line 48: <commons-lang.version>2.6</commons-lang.version> Line 49: <commons-logging.version>1.2</commons-logging.version> please use only slf4j Line 50: <commons-compress.version>1.4.1</commons-compress.version> Line 51: <quartz.version>2.1.2</quartz.version> Line 52: <postgres.jdbc.version>9.1-901-1.jdbc4</postgres.jdbc.version> Line 53: <commons-collections>3.2.1</commons-collections> -- To view, visit http://gerrit.ovirt.org/37886 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondřej Macháček <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
