Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 18: (34 comments) GOOD! Looks very nice. Some more minor comments... :) But I can see the end. https://gerrit.ovirt.org/#/c/37886/18/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 35: ExtensionsToolExecutor.class.getResourceAsStream("arguments.properties"), Line 36: "core" Line 37: ); Line 38: Map<String,Object> argMap = parser.parse(args); Line 39: setupLogger(argMap); so how do we set the logger of the parser :)))) we should probably add a system property to do this as well, move the setupLogger to the earliest point and set the level based on system property, then reset the level based on the arguments. Line 40: if(argMap.containsKey("help")) { Line 41: logger.info( Line 42: String.format( Line 43: "usage: %s%n%n%s%n%n%s%n%n%s", Line 37: ); Line 38: Map<String,Object> argMap = parser.parse(args); Line 39: setupLogger(argMap); Line 40: if(argMap.containsKey("help")) { Line 41: logger.info( this is a good question, should we use logger to print usage and version, or plain stdout. I think that this should go into stdout, as you do want to redirect it and do not add the logger headers such as timestamp, level etc... and you do not want to be effected if logger is redirected to file only. Line 42: String.format( Line 43: "usage: %s%n%n%s%n%n%s%n%n%s", Line 44: parser.getPrefixUsage().replace("@PROGRAM_NAME@", PROGRAM_NAME), Line 45: parser.getPrefixHelp(), Line 42: String.format( Line 43: "usage: %s%n%n%s%n%n%s%n%n%s", Line 44: parser.getPrefixUsage().replace("@PROGRAM_NAME@", PROGRAM_NAME), Line 45: parser.getPrefixHelp(), Line 46: parser.getPrefixUnderlinetext(), all the above can be get out of parser.getUsage(), then all you need to do is replace the substitutions and output the module list. Line 47: getModules(moduleServices) Line 48: ) Line 49: ); Line 50: throw new ExitException(); Line 57: System.getProperty("org.ovirt.engine.exttool.core.packageVersion"), Line 58: System.getProperty("org.ovirt.engine.exttool.core.packageDisplayName") Line 59: ) Line 60: ); Line 61: throw new ExitException(); you can use ExitException(0, "version") so in debug we know why we exited, same to any other exit. Line 62: } Line 63: List<String> moduleOthers = (List<String>) argMap.get("__other__"); Line 64: if (moduleOthers.size() < 1) { Line 65: logger.error(String.format("Please provide module.%n%1$s", getModules(moduleServices))); Line 61: throw new ExitException(); Line 62: } Line 63: List<String> moduleOthers = (List<String>) argMap.get("__other__"); Line 64: if (moduleOthers.size() < 1) { Line 65: logger.error(String.format("Please provide module.%n%1$s", getModules(moduleServices))); please use {} and not String.format when you use logger. module list should be obtain using --help, there is no point in doing this here. but if you want, it is ok, but... it should probably be: ERROR Please provide module name. Available modules: x, y, z or: ERROR Module name is required INFO Available module names: x, y, z please avoid using new line within log messages, as it is very difficult to parse it later. Line 66: throw new ExitException(1); Line 67: } Line 68: String module = moduleOthers.get(0); Line 69: ModuleService moduleService = moduleServices.get(module); Line 64: if (moduleOthers.size() < 1) { Line 65: logger.error(String.format("Please provide module.%n%1$s", getModules(moduleServices))); Line 66: throw new ExitException(1); Line 67: } Line 68: String module = moduleOthers.get(0); maybe if module is "help" we can print usage as well. Line 69: ModuleService moduleService = moduleServices.get(module); Line 70: if (moduleService == null) { Line 71: logger.error(String.format("No such '%1$s' module exists.", module)); Line 72: throw new ExitException(1); Line 70: if (moduleService == null) { Line 71: logger.error(String.format("No such '%1$s' module exists.", module)); Line 72: throw new ExitException(1); Line 73: } Line 74: loadExtensions(moduleService, argMap); loadExtensions after module parse argument? so we avoid doing anything before syntax is correct? otherwise there is no sense in splitting the parse arguments and run. Line 75: moduleService.parseArguments(moduleOthers); Line 76: moduleService.run(); Line 77: } catch(ExitException e) { Line 78: logger.debug(e.getMessage(), e); Line 80: } catch (Throwable t) { Line 81: String message = t.getMessage() != null ? t.getMessage() : t.getClass().getName(); Line 82: logger.error(message); Line 83: logger.debug(message, t); Line 84: } please debug the exitStatus. Line 85: System.exit(exitStatus); Line 86: } Line 87: Line 88: private static void loadExtensions(ModuleService moduleService, Map<String, Object> argMap) { Line 93: if(files == null) { Line 94: files = listFiles( Line 95: ((String) argMap.get("extensions-dir")).replace( Line 96: "@ENGINE_ETC@", Line 97: System.getProperty("org.ovirt.engine.exttool.core.engineEtc") if you put the program name as static, why not align? not important which, but please be consistent. Line 98: ), Line 99: "properties" Line 100: ); Line 101: } Line 144: File dir = new File(directory); Line 145: List<File> propFiles = new ArrayList<>(); Line 146: if (!dir.exists()) { Line 147: logger.warn("Directory {} doesn't exists.", directory); Line 148: return propFiles; please avoid returning in middle of functions. Line 149: } Line 150: File[] dirFiles = dir.listFiles(); Line 151: if (dirFiles != null) { Line 152: for (File file : dirFiles) { Line 146: if (!dir.exists()) { Line 147: logger.warn("Directory {} doesn't exists.", directory); Line 148: return propFiles; Line 149: } Line 150: File[] dirFiles = dir.listFiles(); I think this will return null if directory cannot be found, so no need to add the above check. Line 151: if (dirFiles != null) { Line 152: for (File file : dirFiles) { Line 153: if (file.getName().endsWith(suffix)) { Line 154: propFiles.add(file); Line 162: private static String getModules(Map<String, ModuleService> modules) { Line 163: StringBuilder sb = new StringBuilder(String.format("Available Modules:%n")); Line 164: for(ModuleService entry : modules.values()) { Line 165: sb.append( Line 166: String.format(" %-10s - %s", entry.getName(), entry.getDescription()) I guess you want a new line after each entry? now I also see... if you want to include it in errors and such, you do not want the description, if you want to include in usage you do need... xxx ERROR Please provide module name. Available modules: x, y, z xxx --help Usage: .... Available Modules: x description y description Line 167: ); Line 168: } Line 169: return sb.toString(); Line 170: } https://gerrit.ovirt.org/#/c/37886/18/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 63: public void parseArguments(List<String> moduleArgs) throws Exception { Line 64: moduleArgs.remove(0); Line 65: ParametersParser parser = new ParametersParser( Line 66: getClass().getResourceAsStream("arguments.properties"), Line 67: getName() I think that the prefix of the module core arguments should be the same for all modules within their properties file. it will be easier to distinguish it from the actions. not that important. Line 68: ); Line 69: args.putAll(parser.parse(moduleArgs)); Line 70: if(args.containsKey("help")) { Line 71: printUsage(parser); Line 65: ParametersParser parser = new ParametersParser( Line 66: getClass().getResourceAsStream("arguments.properties"), Line 67: getName() Line 68: ); Line 69: args.putAll(parser.parse(moduleArgs)); why do you need to copy args? what am I missing? logically you should be able to use this reference returned from parser forever. Line 70: if(args.containsKey("help")) { Line 71: printUsage(parser); Line 72: throw new ExitException(); Line 73: } Line 71: printUsage(parser); Line 72: throw new ExitException(); Line 73: } Line 74: Line 75: List<String> actionOthers = (List<String>)args.get("__other__"); I guess "__other__" can be a static constaint within parser :) Line 76: String action = actionOthers.remove(0); Line 77: actionMethod = actions.get(action); Line 78: if(actionMethod == null) { Line 79: throw new IllegalArgumentException( Line 81: ); Line 82: } Line 83: Line 84: parser = new ParametersParser( Line 85: getClass().getResourceAsStream("arguments.properties"), hmmm... just realized, all over, this should be: try(InputStream is = getClass().getResourceAsStream("arguments.properties")) { parser = ... } Line 86: action Line 87: ); Line 88: args.putAll(parser.parse(actionOthers)); Line 89: if(args.containsKey("help")) { Line 84: parser = new ParametersParser( Line 85: getClass().getResourceAsStream("arguments.properties"), Line 86: action Line 87: ); Line 88: args.putAll(parser.parse(actionOthers)); again, I am unsure why you copy. Line 89: if(args.containsKey("help")) { Line 90: printUsage(parser); Line 91: throw new ExitException(); Line 92: } Line 104: parser.getPrefixUsage().replace("@PROGRAM_NAME@", (String)getContext().get(PROGRAM_NAME)), Line 105: parser.getPrefixHelp(), Line 106: parser.getPrefixUnderlinetext() Line 107: ) Line 108: ); should go to stdout, well, I would have put the stdin/stdout/stderr in context by core, but not that important for now. and again, all this can be acquire from the parser, so if we add new fields it will be available to all. all you need to do is print parser.getUsage().replace().replace().replace()... Line 109: } Line 110: Line 111: private void logrecord() { Line 112: ExtensionProxy proxy = ((Map<String, ExtensionProxy>) context.get(EXTENSIONS_MAP)).get( https://gerrit.ovirt.org/#/c/37886/18/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. no need for this underlinetext. but we should cleanup the variable names... first, help is either help or usage, but should be consistent. xxxx [options] xxxx <-- help.usage this program is bla bla <-- help.header --option xxx <-- generated from properties Examples: <-- help.footer xxx --debug aaa in our case: @PROGRAM_NAME@ [options] module [options] oVirt Engine Extension API test tool Options: .... Available modules: logger - bla bla bla Line 24: core.help.footer = oVirt Engine Extension API test tool https://gerrit.ovirt.org/#/c/37886/18/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 1: logger.help.usage.header = @PROGRAM_NAME@ logger [options] action [options] Line 2: logger.help.underlinetext = You can test your logger extension Logger interface test module Line 3: logger.arg.help.name = help Line 4: logger.arg.help.help = show help for logger module Line 5: log-record.arg.logger-name.name = logger-name Line 6: log-record.arg.logger-name.mandatory = true https://gerrit.ovirt.org/#/c/37886/18/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 65: } Line 66: if(!arg.startsWith(LONG_PREFIX)) { Line 67: others.add(arg); Line 68: break; Line 69: } consider switching these... String arg = args.get(0); if (!arg.startsWith(LONG_PREFIX)) { break; } args.remove(0); if (arg.equals(LONG_PREFIX)) { break; } then you do not need to add back what you removed. Line 70: Line 71: String[] argVal = parseArgument(arg.substring(2)); Line 72: String key = argVal[0]; Line 73: String value = argVal[1]; Line 74: Line 75: ParserArgument parserArgument = arguments.get(key); Line 76: if(parserArgument == null) { Line 77: throw new ExitException( Line 78: String.format("Argument '%1$s' is not valid", arg), Invalid argument '%s'? Line 79: 1 Line 80: ); Line 81: } Line 82: if ( Line 92: } Line 93: } Line 94: if(parserArgument.getType() == ArgumentType.has_argument && value == null) { Line 95: throw new ExitException( Line 96: String.format("Argument's '%1$s' value is required", key), Value is required but missing for argument '%s'? Line 97: 1 Line 98: ); Line 99: } Line 100: Object convertedValue = null; Line 102: Matcher m = parserArgument.getMatcher().matcher(value); Line 103: if (!m.find()) { Line 104: throw new ExitException( Line 105: String.format( Line 106: "Argument's '%1$s' value '%2$s' don't match pattern %3$s", key, value, m.pattern() Pattern for argument '%s' does not match, pattern is '%s', value is '%s'? Line 107: ), Line 108: 1 Line 109: ); Line 110: } Line 109: ); Line 110: } Line 111: convertedValue = Util.getObjectValueByString(parserArgument.getConvert(), value); Line 112: } Line 113: Object o = argMap.get(key); can't this be in the else? Line 114: if(!parserArgument.isMutivalue()) { Line 115: argMap.put(key, convertedValue); Line 116: } else { Line 117: if(o == null) { Line 119: objects.add(convertedValue); Line 120: argMap.put(key, objects); Line 121: } else { Line 122: ((List<Object>)o).add(convertedValue); Line 123: } hmmm.... } else { Collection c = (Collection)argMap.get(key); if (c == null) { c = new ArrayList(); argMap.put(key, c); } c.add(convertedValue); } Line 124: } Line 125: } Line 126: mandatory.removeAll(argMap.keySet()); Line 127: if(!mandatory.isEmpty() && !argMap.containsKey("help")) { Line 122: ((List<Object>)o).add(convertedValue); Line 123: } Line 124: } Line 125: } Line 126: mandatory.removeAll(argMap.keySet()); better to copy so that you can call parse() twice? Line 127: if(!mandatory.isEmpty() && !argMap.containsKey("help")) { Line 128: throw new ExitException( Line 129: String.format("Argument(s) '%1$s' required", StringUtils.join(mandatory, ", ")), Line 130: 1 Line 128: throw new ExitException( Line 129: String.format("Argument(s) '%1$s' required", StringUtils.join(mandatory, ", ")), Line 130: 1 Line 131: ); Line 132: } help should not be any interest of the parser. if you do, please add: prefix.arg.xxx.help = true the actual parameter name should not leak here. but it is ok to issue error if --help is given as well... no reason to have this logic. Line 133: others.addAll(args); Line 134: argMap.put("__other__", others); Line 135: Line 136: return argMap; Line 130: 1 Line 131: ); Line 132: } Line 133: others.addAll(args); Line 134: argMap.put("__other__", others); please add public static constant for this within parser and use it all over... Line 135: Line 136: return argMap; Line 137: } Line 138: Line 182: getArgAttrValue(arg, "convert"), Line 183: true, Line 184: Thread.currentThread().getContextClassLoader() Line 185: ) Line 186: ); why can't the argument read it-self out of arg? one way is to convert the properties into map and send it: ParserArgument argument = new ParserArgument(propertiesToMap(arg)); another can be sending the prefix and the entire properties: ParserArgument argument = new ParserArgument(props, prefix); last can use reflection... but this is bad. anyway, the logic of what argument is can be in argument... including how to read it self from the properties. Line 187: } catch (ClassNotFoundException ex) { Line 188: throw new IllegalArgumentException( Line 189: String.format("Can't convert argument: '%1$s'. Please check convert class in properties file.", arg) Line 190: ); Line 257: ); Line 258: } Line 259: Line 260: public String getPrefixHelp() { Line 261: StringBuilder help = new StringBuilder("Options:\n"); please be consistent with %n and \n, %n can be \r\n when required. Line 262: for(String arg : getUsageArguments()) { Line 263: ParserArgument argument = this.arguments.get(arg); Line 264: help.append( Line 265: String.format( Line 272: Line 273: return help.toString(); Line 274: } Line 275: Line 276: public static Properties loadProperties(InputStream resource) { not sure this should be static now Line 277: try ( Line 278: Reader is = new InputStreamReader(resource, Charset.forName("UTF-8")); Line 279: ) { Line 280: Properties prop = new Properties(); Line 284: throw new RuntimeException(ioe); Line 285: } Line 286: } Line 287: Line 288: public static Properties loadProperties(Class<?> clazz, String resource) { I see you do not use this anywhere, so remove? Line 289: return loadProperties(clazz.getResourceAsStream(resource)); Line 290: } https://gerrit.ovirt.org/#/c/37886/18/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 6: arg.metavar = STRING Line 7: arg.multivalue = false Line 8: help.underlinetext = Line 9: help.usage.header = Line 10: help.usage.footer = new line? -- 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: 18 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
