Alon Bar-Lev 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: PACKAGE_NAME-PACKAGE_VERSION (PACKAGE_DISPLAY_NAME) example: ovirt-engine-3.5.4 (rhevm-3.5.4) 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 but this should actually print an error record into log. so it needs to be: logger.error("Module name is required but not specified"); throw new ExitException(1); another option is to log errors in the exception handler, but then you assume that any exit is an error, or you need a new parameter. or if 0 it is info otherwise errror 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) ? 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. 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__") why do you get this __help__ and such out of the args and not out of the parser class using methods? Line 94: ) Line 95: ); Line 96: if(args.containsKey("__modules__")) { Line 97: System.out.println( Line 92: (String)args.get("__help__"), Line 93: (String)args.get("__underlinetext__") Line 94: ) Line 95: ); Line 96: if(args.containsKey("__modules__")) { this again not something I understand.... how did it reach to args? Line 97: System.out.println( Line 98: String.format("Available modules: %s", args.get("__modules__")) Line 99: ); Line 100: } 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 and module description for each. Line 99: ); Line 100: } Line 101: throw new ExitException(); Line 102: } -- 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
