[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands

[Martin Basti]

On 24.03.2016 19:10, Stanislav Laznicka wrote:
> On 03/23/2016 08:13 PM, Martin Basti wrote:
>> [...]
>> Can you please update design
>> http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 (mainly
>> the --suffix option)? Also there are missing clean-ruv and list-ruv
>> commands in design, and fix usage at the bottom.
>>
>> 1)
>> I don't understand this expression
>> +            if dirman_passwd is None or (
>> +               not dirman_passwd and args[0] in cs_enabled_commands):
>>
>> You already tested if subcommand belongs to cs_enabled_commands few
>> lines above, IMO the 'dirman_password is None' expression is enough.
> If I understand it well, when empty password is entered, the program 
> continues and uses Kerberos credentials for some operations. E.g. for 
> list-ruv, if empty password is entered, the command would only display 
> RUVs for domain tree but not for the CA tree no matter if CA is set up 
> or not (in the current state of the patch, after get_ruv refactoring). 
> This here is one possible way around this, although the check for 
> non-empty password might probably just as well be in 
> get_ruv_both_suffixes.

ok
>> 2)
>> +# tuple of commands that work with ca tree and need Directory Manager
>> password
>> +cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")
>>
>> this variable is used only toi detect if dirman passwd is needed, I
>> suggest to rename it to commands_req_dirman_passwd, or something better.
>>
>> 3)
>> Q: Do we need is_cs_set() function?
>> A: Yes!
>>
>> I wanted to give you ultimate NACK, but then I checked how get_ruv code
>> works and I changed my mind.
>>
>> Please write a comment where is_cs_set function is used, why we need
>> extra function instead of catching an exception, possibly you can open a
>> refactoring ticket.
> After the refactoring changes, is_cs_set should not be needed anymore, 
> removed it.
>>
>> Shame:
>> 1)
>> +        if not test_connection(realm, host, options.nolookup) or\
>> Please use parentheses instead of backslash
>>
>> 2)
>> +           args[0] in cs_enabled_commands:
>>
>> +               not dirman_passwd and args[0] in cs_enabled_commands):
>>
>> Indentation is not multiplication of 4
> Shame on me indeed, fixed it.
>>
>> Nitpicks (I don't insist on fixing these):
>> 1)
>> +    if servers.get('ca', None):
>>
>> None is default
>>
>> 2)
>> +        for (netloc, rid) in servers['ca']:
>> parentheses are not needed
>>
>> 3)
>> + print("\t%s: %s" % (netloc, rid))
>> Would be nice to use .format() instead of %
>>
>> Martin^2
>>
>>
>>
>> I changed my mind, ultimate NACK.
>> Please fix get_ruv function, is_cs_set will not help. In case there 
>> are no RUVs but CA is installed, sys.exit there prevents us from 
>> removing RUVs (or any else operation) on domain suffix, and vice versa.
>> I propose to move ticket to 4.4 milestone because I would like to 
>> avoid breaking something in 4.3, as this will hit many places in 
>> ipa-replica-manage.
>>
>> Please provide the refactoring of get_ruv as separate patch a put 
>> these patches on top of it.
>>
>> Martin2
> Did the refactoring. There also seemed to be duplicit code in 
> abort_clean_ruv for some reason, removed it (I hope it does not break 
> anything, but it seemed rather useless). Also had to change the 
> numbers of the patches so that they would apply.

NACK

* ipa-replica-manage refactoring *

1)
Instead of:
-        print("Failed to connect to server %s: %s" % (host, e))
-        sys.exit(1)
+        root_logger.debug("Failed to connect to server {host}: {err}"
+                          .format(host=host, err=e))
+        raise RuntimeError(e)

I expected

-        print("Failed to connect to server %s: %s" % (host, e))
-        sys.exit(1)
+        root_logger.debug(traceback.format_exc())
+        raise RuntimeError("Failed to connect to server {host}: {err}"
+                                      .format(host=host, err=e)))


2)
-        print("No RUV records found.")
-        sys.exit(0)

Here is exit state 0, so we should not raise error.

I think that we should create new nonfatal exception.

3)
-                print("unable to decode: %s" % ruv)
+                root_logger.debug("unable to decode: %s" % ruv)
This may break tests, because the logger logs to stderr, leave it as 
print for now

4)
-    servers = get_ruv(realm, host, dirman_passwd, nolookup)
+    try:
+        servers = get_ruv(realm, host, dirman_passwd, nolookup)
+    except RuntimeError as e:
+        print(e)
+        sys.exit(1)

again we have to print it to stdout for now.


* abort-clean/list/clean-ruv now work for both suffixes *

-            if dirman_passwd is None:
+            if dirman_passwd is None or (
+               not dirman_passwd and args[0] in 
dirman_passwd_req_commands):
                  sys.exit("Directory Manager password required")

Please fix other patch accordingly.
Martin^2