[Freeipa-devel] [PATCH] 857 topology: ipa management commands

[Martin Babinsky]

On 06/03/2015 01:34 PM, Petr Vobornik wrote:
> On 06/03/2015 10:59 AM, Martin Babinsky wrote:
>> On 06/03/2015 10:52 AM, Martin Babinsky wrote:
>>> On 05/26/2015 03:31 PM, Petr Vobornik wrote:
>>>> On 05/26/2015 12:19 PM, Petr Vobornik wrote:
>>>>> this patch is based on top of my patch #856 and tbabej'
>>>>> s 325-9.
>>>>>
>>>>> Obsoletes Ludwig's 0006.
>>>>>
>>>>> ipalib part of topology management
>>>>>
>>>>> Design:
>>>>> - http://www.freeipa.org/page/V4/Manage_replication_topology
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/4302
>>>>>
>>>>>
>>>>
>>>> New version attached:
>>>> - domainlevel_show usage changed to domainlevel_get
>>>> - updated VERSION
>>>> - added more attrs to default_attributes
>>>>
>>>>
>>>
>>> Hi Petr,
>>>
>>> the commands themselves seem to work just fine. I had encountered some
>>> quirks in the underlying topology plugin, but I will address them in a
>>> different thread in order to keep the discussion relevant to the
>>> reviewed patch.
>>>
>>> I have some minor coomments below:
>>>
>>> 1.)
>>>   IPA_API_VERSION_MAJOR=2
>>> -IPA_API_VERSION_MINOR=121
>>> -# Last change: pvoborni - added server-find and server-show
>>> +IPA_API_VERSION_MINOR=122
>>> +# Last change: pvoborni - added topology management commands
>>>
>>> Several people were touching API in the meantime so please double-check
>>> that you have correct VERSION and regenerate API.txt
>
> Patch rebased.
>
>>>
>>> 2.)
>>>
>>> +        Str(
>>> +            'nsds5replicatedattributelist?',
>>> +            cli_name='replattrs',
>>> +            label='Attributes to replicate',
>>> +            doc=_('Attributes that are not replicated to a consumer
>>> server '
>>> +                  'during a fractional update. E.g., `(objectclass=*) '
>>> +                  '$ EXCLUDE accountlockout memberof'),
>>> +        ),
>>> +        Str(
>>> +            'nsds5replicatedattributelisttotal?',
>>> +            cli_name='replattrstotal',
>>> +            label=_('Attributes for total update'),
>>> +            doc=_('Attributes that are not replicated to a consumer
>>> server '
>>> +                  'during a total update. E.g. (objectclass=*) $
>>> EXCLUDE '
>>> +                  'accountlockout'),
>>>
>>> The descriptions of these two options confused me greatly, are these
>>> attributes supposed to be replicated or not, or is there some more
>>> complex logic behind them that I failed to grasp? I am cc'ing Ludwig, he
>>> can probably explain them to us and then we can decide whether we may
>>> alter the descriptions to be less confusing.
>>>
>>> 3.)
>>>
>>> +    takes_params = (
>>> +        Str(
>>> +            'cn',
>>> +            cli_name='name',
>>> +            primary_key=True,
>>> +            label=_('Suffix name'),
>>> +        ),
>>> +        Str(
>>> +            'iparepltopoconfroot',
>>> +            maxlength=255,
>>> +            cli_name='suffix',
>>> +            label=_('Suffix to be managed'),
>>> +            normalizer=lambda value: value.lower(),
>>> +        ),
>>> +    )
>>>
>>> This also confused me at first, I suggest to change the label of
>>> 'iparepltopoconfroot' to something like 'LDAP suffix to be managed' or
>>> 'LDAP subtree to be managed'.
>
> Changed to 'LDAP suffix to be managed'
>
>>>
>>> 4.)
>>>
>>> There is currently no way to rename existing topology segments/suffixes.
>>> In the case of hosts with funky FQDN's (pointing at you, ABC lab), the
>>> segment cn's created during replica installs are mearly impossible to
>>> remember and it would be nice to rename them to something more
>>> manageable. However, this is not related to core functionality and can
>>> be a subject of a separate patch once this gets pushed.
>>>
>>> That's all from my side.
>>>
>>
>> I also forgot to ask what is the expected policy when deleting a
>> non-empty topology suffix. If this is not supported and you have to
>> first remove all segments and then the suffix itself, the
>> 'topologysuffix-del' command should issue an error pointing the user to
>> correct procedure.
>>
>
> Do we have a use case for creation or deletion of topology suffix?
That's a good question.

Anyway, I have noticed couple more things:

1.) it seems that there some of unused imports in topology.py. Please 
investigate whether all of them are really needed.

2.)

+from ipalib.plugins.baseldap import *
+from ipalib.plugins import baseldap

I do not like that starred import at all. Either import the particular 
classes you use (like e.g. in basuser.py), or just leave the second 
import statetement and use the appropriate namespace 
(baseldap.LDAPObject etc.).

3.) there are couple of pep8 complaints, please try to fix them unless 
it impairs readability:

./ipalib/constants.py:121:80: E501 line too long (81 > 79 characters)
./ipalib/plugins/topology.py:72:80: E501 line too long (88 > 79 characters)
./ipalib/plugins/topology.py:73:26: E131 continuation line unaligned for 
hanging indent
./ipalib/plugins/topology.py:73:80: E501 line too long (93 > 79 characters)
./ipalib/plugins/topology.py:103:80: E501 line too long (80 > 79 characters)
./ipalib/plugins/topology.py:111:80: E501 line too long (80 > 79 characters)
./ipalib/plugins/topology.py:207:80: E501 line too long (80 > 79 characters)
./ipalib/plugins/topology.py:232:80: E501 line too long (80 > 79 characters)
./ipalib/plugins/topology.py:269:80: E501 line too long (84 > 79 characters)
./ipalib/plugins/topology.py:278:80: E501 line too long (89 > 79 characters)
./ipalib/plugins/topology.py:363:80: E501 line too long (80 > 79 characters)
./ipalib/plugins/topology.py:375:80: E501 line too long (80 > 79 characters)

-- 
Martin^3 Babinsky