[Freeipa-devel] [PATCH] manage replication topology in the shared tree

Wed May 20 13:07:01 UTC 2015

On 05/20/2015 02:58 PM, Ludwig Krispenz wrote:
>
> On 05/20/2015 02:52 PM, Oleg Fayans wrote:
>> Is this patch to be applied on top of the vanilla upstream tree, or
>> does it require your previous patches applied before?
> it requires the install (0005) and ipa-command (0006) patch as well,
> submitted on 05/12

Patch 0005 can't be applied on top of the new patch 3. Both patches 
contains adding of 70topology.ldif.

Also please clear all trailing whitespaces from patch 3.

$ git am 
freeipa-lkrispen-0003-plugin-part-manage-replication-topology-in-the-shaer.patch
Applying: plugin part - manage replication topology in the shaerd tree
/home/somebody/freeipa/.git/rebase-apply/patch:607: trailing whitespace.
/home/somebody/freeipa/.git/rebase-apply/patch:740: trailing whitespace.
      * find the attrtype and return the corresponding
/home/somebody/freeipa/.git/rebase-apply/patch:742: trailing whitespace.
      */
/home/somebody/freeipa/.git/rebase-apply/patch:745: trailing whitespace.
         /* attr is handling specific direction,
/home/somebody/freeipa/.git/rebase-apply/patch:772: trailing whitespace.
/* two static data structures to hold the
warning: squelched 125 whitespace errors
warning: 130 lines add whitespace errors.

>
>>
>> On 05/19/2015 02:16 PM, Ludwig Krispenz wrote:
>>> Hi,
>>>
>>> here is the latest patch for the plugin part, trying to address all
>>> problems found in the review
>>>
>>> Regards,
>>> Ludwig
>>> PS if you want you can get a separate diff top the last version
>>>
>>>
>>> On 05/12/2015 08:33 AM, Ludwig Krispenz wrote:
>>>> Hi,
>>>>
>>>> I did split the patches, for easier review and to share work on it.
>>>> The attachment contains 4 patches:
>>>> - the ds plugin part as submitted for review
>>>> - the changes to the ds plugin part done after review  (not complete
>>>> yet)
>>>> - the ipa framework part (including Petr's improvements)
>>>> - the install related part
>>>>
>>>> Regards,
>>>> Ludwig
>>>>
>>>> On 04/21/2015 01:09 PM, Petr Vobornik wrote:
>>>>> On 04/21/2015 12:53 PM, Petr Vobornik wrote:
>>>>>> On 04/13/2015 10:56 AM, Ludwig Krispenz wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> in the attachment you find the latest state of the "topology
>>>>>>> plugin", it
>>>>>>> implements what is defined in the design page:
>>>>>>> http://www.freeipa.org/page/V4/Manage_replication_topology (which is
>>>>>>> also waiting for a reviewer)
>>>>>>>
>>>>>>> It contains the plugin itself and  a core of ipa commands to
>>>>>>> manage a
>>>>>>> topology. to be really applicable, some work outside is required,
>>>>>>> eg the
>>>>>>> management of the domain level and a decision where the binddn group
>>>>>>> should be maintained.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ludwig
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> I've looked at the python part, mostly because I want to start
>>>>>> with POC
>>>>>> of Web UI for topology.
>>>>>>
>>>>>> topology.py is clearly still a work in progress. I've reflected
>>>>>> following comments into a patch to speed things up.
>>>>>>
>>>>>> What's in the patch:
>>>>>>
>>>>>> 1. git am complains about trailing whitespaces
>>>>>>
>>>>>> 2. pep8 check produces quite a lot of issues. New code should be
>>>>>> almost
>>>>>> with any (`E501 line too long` is not a hard rule)
>>>>>>     `git diff HEAD~1 -U0 | pep8 --diff`
>>>>>>
>>>>>> 3. some typos
>>>>>>
>>>>>> 4. A lot of unused imports
>>>>>>
>>>>>> 5. Option name --sname for 'Segment identifier' is not very
>>>>>> friendly. I
>>>>>> don't see any examples of command options in the design notes.
>>>>>>
>>>>>> 6. NO_UPG_MAGIC - leftover from other plugin?
>>>>>>
>>>>>> 7. suffix object has labels from segment
>>>>>>
>>>>>> 8. IPA framework has a support for nested object. Key is setting
>>>>>> `parent_object = 'topologysuffix'` in topologysegment object.
>>>>>>
>>>>>> 9. repl_agmt_attrs could be in topologysegment takes_params.
>>>>>>
>>>>>> 10. missing various CRUD commands like topologysuffix-find and
>>>>>> topologysuffix-show commands.
>>>>>>
>>>>>> Whats missing, not fixed:
>>>>>> 1. last 2 lines of VERSION file are not updated
>>>>>>
>>>>>> 2. Mixed terminology. Somewhere is used suffix and somewhere
>>>>>> replication
>>>>>> area or just area.
>>>>>>
>>>>>> 3. Validation
>>>>>> - suffix should check for dn
>>>>>> - existence of both ends of a segment
>>>>>>
>>>>>> 4. print of segments in suffix-show needs to be improved or removed
>>>>>>
>>>>>> To discuss:
>>>>>> a) Do params in topologysegment have to have a maxlength set?
>>>>>>
>>>>>> b) Terminology has to be united. Segments are nested in suffix but
>>>>>> sometimes are called areas and suffix is 'the suffix'. User might be
>>>>>> confused. E.g. shouldn't the object be named a topologyarea
>>>>>> instead of
>>>>>> topologysuffix?
>>>>>>
>>>>>> c) I've added all missing CRUD commands. Are there any which we don't
>>>>>> want there, or want to restrict them. E.g. I can imagine that
>>>>>> deleting a
>>>>>> suffix should be prevented if it contains any segments (or it has
>>>>>> to be
>>>>>> forced (--force option))
>>>>>>
>>>>>> d) Do we want to print segments in suffix-show?
>>>>>>
>>>>>> e) Mainly for Honza: I've added --show-segments option to suffix-show
>>>>>> which defaults to True. I don't like the behavior of CLI, which
>>>>>> asks to
>>>>>> confirm the value all the time. My intention was to have it there by
>>>>>> default, but also allow to disable it by --show-segments=False. I
>>>>>> don't
>>>>>> want to add it as Flag (--hide-segments) since it restricts
>>>>>> versatility.
>>>>>> I would like to see an optional flag which would be filled by default
>>>>>> value if not explicitly defined and CLI would not ask for the
>>>>>> option value.
>>>>>>
>>>>>>
>>>>>
>>>>> Also it would be better to split the work into more patches. E.g.
>>>>> DS plugin, installation, python plugin. So ds plugin review could
>>>>> be separated from the python part.
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>> --
>> Oleg Fayans
>> Quality Engineer
>> FreeIPA team
>> RedHat.
>
>
>
>

-- 
Petr Vobornik