[Freeipa-devel] [PATCH] 857 topology: ipa management commands
Martin Babinsky
mbabinsk at redhat.com
Wed Jun 3 15:28:02 UTC 2015
On 06/03/2015 03:53 PM, Petr Vobornik wrote:
> On 06/03/2015 02:38 PM, Martin Babinsky wrote:
>> 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.
>
> Fixed
>
>>
>> 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.).
>
> Fixed
>
>>
>> 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)
>
> won't fix
>
>> ./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)
>
> fixed
>
>> ./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)
>>
> won't fix
>
ACK
--
Martin^3 Babinsky
More information about the Freeipa-devel
mailing list