Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 17: (16 comments) https://gerrit.ovirt.org/#/c/37886/17/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java: Line 48: } Line 49: if(argMap.containsKey("version")) { Line 50: logger.info( Line 51: String.format( Line 52: "%1$s - %2$s (%3$s)", %1$s-%2$s (%3$s), notice the space Line 53: System.getProperty("org.ovirt.engine.exttool.core.packageName"), Line 54: System.getProperty("org.ovirt.engine.exttool.core.packageVersion"), Line 55: System.getProperty("org.ovirt.engine.exttool.core.packageDisplayName") Line 56: ) Line 70: } Line 71: loadExtensions(moduleService, argMap); Line 72: moduleService.parseArguments( Line 73: moduleService.parseArguments(moduleOthers) Line 74: ); why is double parsing here? you should provided the others to the module to parse, it should ignore its name if not important to it and parse arguments. Line 75: moduleService.run(); Line 76: } catch(ExitException e) { Line 77: if(e.getMessage() != null) { Line 78: if(e.getExitCode() > 0) { Line 78: if(e.getExitCode() > 0) { Line 79: logger.error(e.getMessage()); Line 80: } else { Line 81: logger.info(e.getMessage()); Line 82: } which makes me wounder why do we need the message within this exception... why not info/error before throwing it and only use the message for debug. Line 83: } Line 84: exitStatus = e.getExitCode(); Line 85: } catch (Throwable t) { Line 86: String message = t.getMessage() != null ? t.getMessage() : t.getClass().getName(); Line 155: return propFiles; Line 156: } Line 157: Line 158: private static String getModules(Map<String, ModuleService> modules) { Line 159: StringBuilder sb = new StringBuilder("Available Modules:\n"); if you using %n in usage, then we should be consistent... Line 160: for(ModuleService entry : modules.values()) { Line 161: sb.append(String.format(" %-10s - %s", entry.getName(), entry.getDescription())); Line 162: } Line 163: return sb.toString(); https://gerrit.ovirt.org/#/c/37886/17/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 8: Line 9: public interface ModuleService { Line 10: Line 11: public static final ExtKey EXTENSIONS_MAP = new ExtKey("GLOBAL_EXTENSIONS_MAP", Map/*<ExtMap>*/.class, "78680401-40cf-4bae-a6ee-7e8c204a9010"); Line 12: public static String PROGRAM_NAME ="test";// System.getProperty("org.ovirt.engine.exttool.core.packageName"); why is it here and not within core? you can add it to the context to be available at module. Line 13: Line 14: public String getName(); Line 15: Line 16: public String getDescription(); Line 14: public String getName(); Line 15: Line 16: public String getDescription(); Line 17: Line 18: public Map<String, Object> getArguments(); I am still unsure why this is required. Line 19: Line 20: public boolean doHelp(); Line 21: Line 22: public void setContext(ExtMap context); https://gerrit.ovirt.org/#/c/37886/17/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/LoggerServiceImpl.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/LoggerServiceImpl.java: Line 18: public class LoggerServiceImpl implements ModuleService { Line 19: Line 20: private static final org.slf4j.Logger logger = LoggerFactory.getLogger(LoggerServiceImpl.class); Line 21: Line 22: private String prefix; not sure why this is a member. Line 23: private ExtMap context; Line 24: private Map<String, Object> args = new HashMap<>(); Line 25: private Map<String, Runnable> actions = new HashMap<>(); Line 26: { Line 72: public List<String> parseArguments(List<String> argsList) throws Exception { Line 73: prefix = argsList.remove(0); Line 74: ParametersParser parser = new ParametersParser( Line 75: getClass().getResourceAsStream("arguments.properties"), Line 76: prefix this should be constant Line 77: ); Line 78: args.putAll(parser.parse(argsList)); Line 79: if(args.containsKey("help")) { Line 80: logger.info( Line 87: ); Line 88: throw new ExitException(); Line 89: } Line 90: Line 91: return (List<String>)args.get("__other__"); why not store it as member? Line 92: } Line 93: Line 94: @Override Line 95: public void run() throws Exception { Line 92: } Line 93: Line 94: @Override Line 95: public void run() throws Exception { Line 96: Runnable actionMethod = actions.get(prefix); the action should be parsed in this module. Line 97: if(actionMethod == null) { Line 98: throw new IllegalArgumentException( Line 99: String.format("No such action '%1$s' exists for module '%2$s'", prefix, getName()) Line 100: ); Line 95: public void run() throws Exception { Line 96: Runnable actionMethod = actions.get(prefix); Line 97: if(actionMethod == null) { Line 98: throw new IllegalArgumentException( Line 99: String.format("No such action '%1$s' exists for module '%2$s'", prefix, getName()) shouldn't this check be in the parseArguments? why defer it to run? Line 100: ); Line 101: } Line 102: Line 103: actionMethod.run(); https://gerrit.ovirt.org/#/c/37886/17/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/arguments.properties File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/arguments.properties: Line 19: core.arg.help.name = help Line 20: core.arg.help.help = show test tool help Line 21: core.arg.version.name = version Line 22: core.arg.version.help = show version of test tool Line 23: core.help.underlinetext = You can use this tool to test your extension. footer? oVirt Engine Extension API test tool? Line 20: core.arg.help.help = show test tool help Line 21: core.arg.version.name = version Line 22: core.arg.version.help = show version of test tool Line 23: core.help.underlinetext = You can use this tool to test your extension. Line 24: core.help.usage.header = @PROGRAM_NAME@ [options] module @PROGRAM_NAME@ [options] module [options] https://gerrit.ovirt.org/#/c/37886/17/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/arguments.properties File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/arguments.properties: Line 22: log-record.help.usage.header = @PROGRAM_NAME@ logger logrecord [options] Line 23: logger.arg.help.name = help Line 24: logger.arg.help.help = show help for logger module Line 25: logger.help.underlinetext = You can test your logger extension Line 26: logger.help.usage.header = @PROGRAM_NAME@ logger [options] action [options] logger -> action|module -> so it will be the same at all modules we add. please also reorder so it will be first. https://gerrit.ovirt.org/#/c/37886/17/backend/manager/modules/uutils/src/main/resources/org/ovirt/engine/core/uutils/cli/parser/defaults.properties File backend/manager/modules/uutils/src/main/resources/org/ovirt/engine/core/uutils/cli/parser/defaults.properties: Line 5: arg.matcher = .* Line 6: arg.metavar = STRING Line 7: arg.multivalue = false Line 8: help.underlinetext = Line 9: help.usage.header = new line please https://gerrit.ovirt.org/#/c/37886/17/packaging/bin/ovirt-engine-extensions-tool.sh File packaging/bin/ovirt-engine-extensions-tool.sh: Line 24: -Djboss.modules.write-indexes=false \ Line 25: -Dorg.ovirt.engine.exttool.core.programName="${0}" \ Line 26: -Dorg.ovirt.engine.exttool.core.packageName="${PACKAGE_NAME}" \ Line 27: -Dorg.ovirt.engine.exttool.core.packageVersion="${PACKAGE_VERSION}" \ Line 28: -Dorg.ovirt.engine.exttool.core.packageDisplayName="${PACKAGE_DISPLAY_NAME}" \ etc is missing Line 29: -jar "${JBOSS_HOME}/jboss-modules.jar" \ Line 30: -dependencies org.ovirt.engine.core.extension-tool \ Line 31: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \ -- To view, visit https://gerrit.ovirt.org/37886 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409 Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondřej Macháček <machacek.on...@gmail.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ondra Machacek <omach...@redhat.com> Gerrit-Reviewer: Ondřej Macháček <machacek.on...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer <mta...@redhat.com> 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
