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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky