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

[Petr Vobornik]

On 03/30/2016 09:37 AM, Stanislav Laznicka wrote:
> On 03/24/2016 07:10 PM, 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.
>>> 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.
>>
>>
> Self NACK. As I was updating the design today, I found out I omitted the
> information that abort-clean-ruv should now be called with --force by default.
> Updated the arguments of the abort call + man page in the patchset.
>

I made a mistake in the design page. By --force I actually meant to use 
`replica-force-cleaning: yes"` as written in 
https://fedorahosted.org/freeipa/ticket/5396 (which means the relevant 
ticket in design is wrong).

But #5396 is especially important for clean-dangling ruvs sub command.



-- 
Petr Vobornik