Ondřej Macháček has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 16: (7 comments) https://gerrit.ovirt.org/#/c/37886/16/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 42: "%1$s - %2$s (%3$s)", Line 43: System.getProperty("org.ovirt.engine.exttool.core.packageDisplayName"), Line 44: System.getProperty("org.ovirt.engine.exttool.core.packageVersion"), Line 45: programName Line 46: ) > should be: Done Line 47: ); Line 48: throw new ExitException(); Line 49: } Line 50: List<String> moduleOthers = (List<String>) argMap.get("__other__"); Line 48: throw new ExitException(); Line 49: } Line 50: List<String> moduleOthers = (List<String>) argMap.get("__other__"); Line 51: if (moduleOthers.size() < 1) { Line 52: throw new ExitException("Please provide module.", 0); > this is an error, exit should be 1 Done Line 53: } Line 54: String module = moduleOthers.remove(0); Line 55: ModuleService moduleService = moduleServices.get(module); Line 56: if (moduleService == null) { Line 68: moduleService.parseArguments(actionOthers, action); Line 69: if(moduleService.doHelp()) { Line 70: printHelpAndExit(moduleService.getArguments()); Line 71: } Line 72: moduleService.run(action); > exitStatus = moduleService.run(action) ? There is ExitException for this. Anyway Runnable run is declared as void run() Line 73: } catch(ExitException e) { Line 74: if(e.getMessage() != null) { Line 75: System.out.println(e.getMessage()); Line 76: } Line 78: } catch (Throwable t) { Line 79: String message = t.getMessage() != null ? t.getMessage() : t.getClass().getName(); Line 80: logger.error(message); Line 81: logger.debug(message, t); Line 82: t.printStackTrace(); > this is provided by debug, no? please do not use anything but logger. Done Line 83: } Line 84: System.exit(exitStatus); Line 85: } Line 86: Line 89: String.format( Line 90: "usage: %s%n%n%s%n%n%s", Line 91: ((String)args.get("__usage__")).replace("@PROGRAM_NAME@", programName), Line 92: (String)args.get("__help__"), Line 93: (String)args.get("__underlinetext__") > I do not see a reason to lose the reference. Reworked.. Line 94: ) Line 95: ); Line 96: if(args.containsKey("__modules__")) { Line 97: System.out.println( Line 94: ) Line 95: ); Line 96: if(args.containsKey("__modules__")) { Line 97: System.out.println( Line 98: String.format("Available modules: %s", args.get("__modules__")) > we should enumerate the services not the args, and best to have module name Done Line 99: ); Line 100: } Line 101: throw new ExitException(); Line 102: } https://gerrit.ovirt.org/#/c/37886/16/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 13: public String getName(); Line 14: Line 15: public String getDescription(); Line 16: Line 17: public Map<String, Object> getArguments(); > but you do not actually need this, we agreed to just display the module lis removed. Line 18: Line 19: public boolean doHelp(); Line 20: Line 21: public void setContext(ExtMap context); -- 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: 16 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
