Alon Bar-Lev has posted comments on this change. Change subject: tools: Fix command line args in engine-manage-domains ......................................................................
Patch Set 1: (5 comments) this is much closer to what I imagined! only 700 lines and counting (down) :))) http://gerrit.ovirt.org/#/c/23842/1/backend/manager/tools/src/main/java/org/ovirt/engine/core/domains/ManageDomainsArguments.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/domains/ManageDomainsArguments.java: Line 183: } Line 184: Line 185: if (!isValidAction(args[0])) { Line 186: throw new ManageDomainsResult(ManageDomainsResultEnum.INVALID_ACTION, args[0]); Line 187: } I suggest --action= to make it consistent not go based on positional. Line 188: Line 189: String action = args[0]; Line 190: argMap.put(ARG_ACTION, action); Line 191: Line 193: if (args.length > 1) { Line 194: // entered more args then just action Line 195: try { Line 196: ExtendedCliParser parser = new ExtendedCliParser(); Line 197: parsed = parser.parse(Arrays.copyOfRange(args, 1, args.length)); why not provide parser with start index and length? if not our code, then ok... Line 198: } catch (IllegalArgumentException ex) { Line 199: throw new ManageDomainsResult(ManageDomainsResultEnum.INVALID_ARGUMENT, ex.getMessage()); Line 200: } Line 201: } else { Line 198: } catch (IllegalArgumentException ex) { Line 199: throw new ManageDomainsResult(ManageDomainsResultEnum.INVALID_ARGUMENT, ex.getMessage()); Line 200: } Line 201: } else { Line 202: parsed = Collections.<String, String> emptyMap(); why not set it to empty map and avoid conditional? Line 203: } Line 204: Line 205: if (!parsed.isEmpty()) { Line 206: if (parsed.containsKey(ARG_HELP)) { Line 205: if (!parsed.isEmpty()) { Line 206: if (parsed.containsKey(ARG_HELP)) { Line 207: // print help Line 208: argMap.put(ARG_HELP, null); Line 209: return; raise ManageDomainResult? Line 210: } Line 211: Line 212: // config file can be entered for all actions Line 213: moveArgsValues(parsed, ARG_CONFIG_FILE); Line 221: checkRequiredArgsValue(ARG_DOMAIN, ARG_PROVIDER, ARG_USER, ARG_PASSWORD_FILE, Line 222: ARG_CHANGE_PASSWORD_URL); Line 223: Line 224: } else if (ACTION_EDIT.equals(action)) { Line 225: moveArgsValues(parsed, why do we need move? why can't we have a map with: ACTION_EDIT: [ARG_DOMAIN:required, ARG_PROVIDER:optional, ...] ACTION_DELETE: [...] then validate that you have only these using one call? Line 226: ARG_DOMAIN, ARG_PROVIDER, ARG_USER, ARG_PASSWORD_FILE, ARG_LDAP_SERVERS, Line 227: ARG_ADD_PERMISSIONS, ARG_CHANGE_PASSWORD_URL); Line 228: checkRequiredArgs(ARG_DOMAIN); Line 229: checkRequiredArgsValue(ARG_DOMAIN, ARG_PROVIDER, ARG_USER, ARG_PASSWORD_FILE, -- To view, visit http://gerrit.ovirt.org/23842 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1296958a54d8b802e0d6fb59f1f75cc608e75818 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
