Alon Bar-Lev has posted comments on this change. Change subject: manage domains: convert to new parameters parser ......................................................................
Patch Set 2: (5 comments) General note, a boolean parameter of: --xxx should put true in args, while default is false. much nicer than checking if key exists or not. so like we have multi value we can also have boolean flags. https://gerrit.ovirt.org/#/c/39972/2/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/tools/ManageDomains.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/tools/ManageDomains.java: Line 643: if (args.containsKey(ARG_LDAP_SERVERS)) { Line 644: setLdapServersPerDomain(domainName, ldapServersEntry, StringUtils.join(ldapServers, ",")); Line 645: } Line 646: LdapProviderType ldapProviderType = null; Line 647: if (args.containsKey(ARG_PROVIDER)) { isn't provider mandatory? Line 648: ldapProviderType = LdapProviderType.valueOfIgnoreCase( Line 649: (String)args.get(ManageDomainsArguments.ARG_PROVIDER) Line 650: ); Line 651: } Line 848: passwords.getValueForDomain(domain), Line 849: userGuid, LdapProviderType.valueOf(ldapProviderType.getValueForDomain(domain)), domainLdapServers); Line 850: if (!result.isSuccessful()) { Line 851: if (isValidate || ((domainName != null) && !domain.equals(domainName))) { Line 852: if (args.containsKey(ARG_REPORT)) { this should be boolean with default false I guess, so check its value and not existence. Line 853: System.out.println("WARNING, domain: " + domain + " may not be functional: " Line 854: + result.getDetailedMessage()); Line 855: } else { Line 856: throw result; Line 981: Line 982: configurationProvider.setConfigValue(ConfigValues.LDAPProviderTypes, Line 983: ldapProviderTypeEntry); Line 984: Line 985: if (args.containsKey(ARG_CHANGE_PASSWORD_MSG)) { this is strange... if argument is provided we put a different value(?) Line 986: configurationProvider.setConfigValue(ConfigValues.ChangePasswordMsg, changePasswordUrlEntry); Line 987: } Line 988: } Line 989: Line 999: } Line 1000: Line 1001: //Prompt warning about last domain only if not "force delete", as using Line 1002: //the force delete option should remove with no confirmation/warning Line 1003: if (domainNameEntry.getDomainNames().size() == 1 && !args.containsKey(ARG_FORCE)) { the force will be default False, so you can just get it. Line 1004: System.out.println(String.format(WARNING_ABOUT_TO_DELETE_LAST_DOMAIN, domainName)); Line 1005: } Line 1006: Line 1007: if(!args.containsKey(ARG_FORCE) && !confirmDeleteDomain(domainName)) { https://gerrit.ovirt.org/#/c/39972/2/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/tools/ManageDomainsArguments.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/tools/ManageDomainsArguments.java: Line 145 Line 146 Line 147 Line 148 Line 149 user? -- To view, visit https://gerrit.ovirt.org/39972 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I983b60824e31f279df0fe11c8adeb34b16f56e6a Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondra Machacek <omach...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ondřej Macháček <machacek.on...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org 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
