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

[Martin Babinsky]

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
>
> 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'.
>
> 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.

-- 
Martin^3 Babinsky