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

[Ludwig Krispenz]

On 05/20/2015 03:07 PM, Petr Vobornik wrote:
> 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.
ok, this was my mistake when splitting the original patch, it should 
only be in the plugin part
the trailing spaces in most cases are leftovers from the request to make 
lines shorter, I will fix it for a new version
>
> 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.
>>
>>
>>
>>
>
>