[Freeipa-devel] [PATCH] 857 topology: ipa management commands
Petr Vobornik
pvoborni at redhat.com
Wed Jun 3 13:53:37 UTC 2015
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
--
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0857-3-topology-ipa-management-commands.patch
Type: text/x-patch
Size: 27673 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150603/f079a80c/attachment.bin>
More information about the Freeipa-devel
mailing list