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

[Stanislav Laznicka]

On 03/30/2016 09:54 AM, Petr Vobornik wrote:
> 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.
>
>
I updated the design accordingly, then. The 'almost' original patchset 
can therefore be used.

Note: clean-dangling-ruv now uses the --force option on clean-ruv by 
default. It may or may not need to be updated later according to how 
#5396 is implemented.