Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 6: (5 comments) https://gerrit.ovirt.org/#/c/37886/6/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 46: throw new IllegalArgumentException( Line 47: String.format("No such '%1$s' module exists. To list existing modules use --help argument.", module) Line 48: ); Line 49: } Line 50: if(moduleService.parseArguments(others)) { > I wanted to parse help in module, but you said it should be here. but the story is not right... parseArguments can return true/false for something that is expected, like success/error or has-parameters/does-not-have-parameters, return true/false for help is unexpected while reading code. Line 51: System.out.println(module + " help"); // TODO: help printer Line 52: return; Line 53: } Line 54: https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java: Line 33: public void run() { Line 34: logrecord(); Line 35: } Line 36: } Line 37: ); > Then logrecord method have to be static as well. and everything that is re not sure why static inner class is a solution, I will check it out. Line 38: } Line 39: Line 40: @Override Line 41: public String getName() { Line 50: @Override Line 51: public boolean parseArguments(List<String> argsList) throws Exception { Line 52: ParametersParser parser = new ParametersParser( Line 53: CoreUtil.loadProperties(getClass(), "arguments.properties"), Line 54: CoreUtil.loadProperties(getClass(), "defaults.properties") > Maybe I incorrectly understood your previous comments in previous patch. the default is for the entire parser, not per module... this to reduce the parametersparser logic. Line 55: ); Line 56: args = parser.parse(argsList); Line 57: Line 58: return args.containsKey("help"); https://gerrit.ovirt.org/#/c/37886/6/backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/Base.java File backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/Base.java: > To add new key with Extension_map ? Or I don't understand question. not sure I understand why do you need this new key... and if this is something that should be public to all usages of api. Line 1: package org.ovirt.engine.api.extensions; Line 2: Line 3: import java.util.Collection; Line 4: import java.util.Map; https://gerrit.ovirt.org/#/c/37886/6/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParametersParser.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParametersParser.java: Line 23: private Map<String, ParserArgument> arguments; Line 24: private Set<String> mandatory; Line 25: private List<String> defaults; Line 26: Line 27: public ParametersParser(Properties properties, Properties defaultPropeties) { > Not sure I understand .. these files are incompatible... they should be compatible, only with different prefix. Line 28: this.arguments = new HashMap<>(); Line 29: this.mandatory = new HashSet<>(); Line 30: this.defaults = new ArrayList<>(); Line 31: parseProperties(properties, defaultPropeties); -- 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: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondřej Macháček <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Ondra Machacek <[email protected]> Gerrit-Reviewer: Ondřej Macháček <[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
