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

Thu May 21 10:55:54 UTC 2015

On 05/20/2015 05:40 PM, Ludwig Krispenz wrote:
> please find new versions of patches 0003 and 0005 for  the topology 
> plugin.
>
> the ds plugin patch includes
> - changes to match domain level patch
> - remove trailing white spaces
> - use proper oids for topology schema
>
> the install patch
> - has the  70topology.ldif removed

Hello Ludwig,

I failed to do 'git am patch-0003', it returned a strange "Patch format 
detection failed.".
However I was able to do 'git apply' and to review it.

I focus on the points discussed during previous reviews.
The patch is looking good to me. ACK

thanks
thierry
>
> both patches can now be applied
>
> On 05/20/2015 03:40 PM, Ludwig Krispenz wrote:
>>
>> 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.
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150521/5a1b801a/attachment.htm>