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

[Martin Basti]

On 06.04.2016 14:04, Stanislav Laznicka wrote:
> On 03/30/2016 04:52 PM, Martin Basti wrote:
>> 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)))
> Should be fixed now.
>>
>> 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.
> Made a new nonfatal error exception, then. This step was a bit 
> controversial when it comes to get_ruv_both_suffixes because it needs 
> to catch both this new exception and a RuntimeError exception for both 
> trees. As the main idea here was not to stop if either tree is missing 
> (resulting in RuntimeError in get_ruv) or contains no records 
> (NonFatalError), it is only printed that something bad may have 
> happened (or it's debug-logged in case of nonfatal errors). In the 
> end, only NonFatalError exception is raised if no records were found 
> for whatever reason resulting in the script always returning 0.
>>
>> 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.
> 3), 4) done, although it might be better to log to stderr from patch 
> number 27 and on since the default behavior is changed anyway.
>>
>> * 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")
> Done.
>>
>> Please fix other patch accordingly.
>> Martin^2
>

1)
+class NonFatalError(Exception):
+    pass

IMO we should be more specific and call it NoRUVsFound[Exception]

2)
Not sure if this i sstill refactoring, it should be separate patch
-            if dirman_passwd is None:
+            if dirman_passwd is None or (
+               not dirman_passwd and args[0] in 
dirman_passwd_req_commands):

3)
+def get_ruv_both_suffixes

I think in case where both suffixes returns RuntimeError we should raise 
RuntimeError too which results into exit with error code.