<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 05/20/2015 05:40 PM, Ludwig Krispenz
wrote:<br>
</div>
<blockquote cite="mid:555CAADB.1030306@redhat.com" type="cite">please
find new versions of patches 0003 and 0005 for the topology
plugin.
<br>
<br>
the ds plugin patch includes
<br>
- changes to match domain level patch
<br>
- remove trailing white spaces
<br>
- use proper oids for topology schema
<br>
<br>
the install patch
<br>
- has the 70topology.ldif removed
<br>
</blockquote>
<br>
Hello Ludwig,<br>
<br>
I failed to do 'git am patch-0003', it returned a strange "Patch
format detection failed.".<br>
However I was able to do 'git apply' and to review it.<br>
<br>
I focus on the points discussed during previous reviews. <br>
The patch is looking good to me. ACK<br>
<br>
thanks<br>
thierry<br>
<blockquote cite="mid:555CAADB.1030306@redhat.com" type="cite">
<br>
both patches can now be applied
<br>
<br>
On 05/20/2015 03:40 PM, Ludwig Krispenz wrote:
<br>
<blockquote type="cite">
<br>
On 05/20/2015 03:07 PM, Petr Vobornik wrote:
<br>
<blockquote type="cite">On 05/20/2015 02:58 PM, Ludwig Krispenz
wrote:
<br>
<blockquote type="cite">
<br>
On 05/20/2015 02:52 PM, Oleg Fayans wrote:
<br>
<blockquote type="cite">Is this patch to be applied on top
of the vanilla upstream tree, or
<br>
does it require your previous patches applied before?
<br>
</blockquote>
it requires the install (0005) and ipa-command (0006) patch
as well,
<br>
submitted on 05/12
<br>
</blockquote>
<br>
Patch 0005 can't be applied on top of the new patch 3. Both
patches contains adding of 70topology.ldif.
<br>
</blockquote>
ok, this was my mistake when splitting the original patch, it
should only be in the plugin part
<br>
the trailing spaces in most cases are leftovers from the request
to make lines shorter, I will fix it for a new version
<br>
<blockquote type="cite">
<br>
Also please clear all trailing whitespaces from patch 3.
<br>
<br>
$ git am
freeipa-lkrispen-0003-plugin-part-manage-replication-topology-in-the-shaer.patch<br>
Applying: plugin part - manage replication topology in the
shaerd tree
<br>
/home/somebody/freeipa/.git/rebase-apply/patch:607: trailing
whitespace.
<br>
/home/somebody/freeipa/.git/rebase-apply/patch:740: trailing
whitespace.
<br>
* find the attrtype and return the corresponding
<br>
/home/somebody/freeipa/.git/rebase-apply/patch:742: trailing
whitespace.
<br>
*/
<br>
/home/somebody/freeipa/.git/rebase-apply/patch:745: trailing
whitespace.
<br>
/* attr is handling specific direction,
<br>
/home/somebody/freeipa/.git/rebase-apply/patch:772: trailing
whitespace.
<br>
/* two static data structures to hold the
<br>
warning: squelched 125 whitespace errors
<br>
warning: 130 lines add whitespace errors.
<br>
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">
<br>
On 05/19/2015 02:16 PM, Ludwig Krispenz wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
here is the latest patch for the plugin part, trying to
address all
<br>
problems found in the review
<br>
<br>
Regards,
<br>
Ludwig
<br>
PS if you want you can get a separate diff top the last
version
<br>
<br>
<br>
On 05/12/2015 08:33 AM, Ludwig Krispenz wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
I did split the patches, for easier review and to
share work on it.
<br>
The attachment contains 4 patches:
<br>
- the ds plugin part as submitted for review
<br>
- the changes to the ds plugin part done after review
(not complete
<br>
yet)
<br>
- the ipa framework part (including Petr's
improvements)
<br>
- the install related part
<br>
<br>
Regards,
<br>
Ludwig
<br>
<br>
On 04/21/2015 01:09 PM, Petr Vobornik wrote:
<br>
<blockquote type="cite">On 04/21/2015 12:53 PM, Petr
Vobornik wrote:
<br>
<blockquote type="cite">On 04/13/2015 10:56 AM,
Ludwig Krispenz wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
in the attachment you find the latest state of
the "topology
<br>
plugin", it
<br>
implements what is defined in the design page:
<br>
<a class="moz-txt-link-freetext" href="http://www.freeipa.org/page/V4/Manage_replication_topology">http://www.freeipa.org/page/V4/Manage_replication_topology</a>
(which is
<br>
also waiting for a reviewer)
<br>
<br>
It contains the plugin itself and a core of ipa
commands to
<br>
manage a
<br>
topology. to be really applicable, some work
outside is required,
<br>
eg the
<br>
management of the domain level and a decision
where the binddn group
<br>
should be maintained.
<br>
<br>
Thanks,
<br>
Ludwig
<br>
<br>
<br>
</blockquote>
<br>
I've looked at the python part, mostly because I
want to start
<br>
with POC
<br>
of Web UI for topology.
<br>
<br>
topology.py is clearly still a work in progress.
I've reflected
<br>
following comments into a patch to speed things
up.
<br>
<br>
What's in the patch:
<br>
<br>
1. git am complains about trailing whitespaces
<br>
<br>
2. pep8 check produces quite a lot of issues. New
code should be
<br>
almost
<br>
with any (`E501 line too long` is not a hard rule)
<br>
`git diff HEAD~1 -U0 | pep8 --diff`
<br>
<br>
3. some typos
<br>
<br>
4. A lot of unused imports
<br>
<br>
5. Option name --sname for 'Segment identifier' is
not very
<br>
friendly. I
<br>
don't see any examples of command options in the
design notes.
<br>
<br>
6. NO_UPG_MAGIC - leftover from other plugin?
<br>
<br>
7. suffix object has labels from segment
<br>
<br>
8. IPA framework has a support for nested object.
Key is setting
<br>
`parent_object = 'topologysuffix'` in
topologysegment object.
<br>
<br>
9. repl_agmt_attrs could be in topologysegment
takes_params.
<br>
<br>
10. missing various CRUD commands like
topologysuffix-find and
<br>
topologysuffix-show commands.
<br>
<br>
Whats missing, not fixed:
<br>
1. last 2 lines of VERSION file are not updated
<br>
<br>
2. Mixed terminology. Somewhere is used suffix and
somewhere
<br>
replication
<br>
area or just area.
<br>
<br>
3. Validation
<br>
- suffix should check for dn
<br>
- existence of both ends of a segment
<br>
<br>
4. print of segments in suffix-show needs to be
improved or removed
<br>
<br>
To discuss:
<br>
a) Do params in topologysegment have to have a
maxlength set?
<br>
<br>
b) Terminology has to be united. Segments are
nested in suffix but
<br>
sometimes are called areas and suffix is 'the
suffix'. User might be
<br>
confused. E.g. shouldn't the object be named a
topologyarea
<br>
instead of
<br>
topologysuffix?
<br>
<br>
c) I've added all missing CRUD commands. Are there
any which we don't
<br>
want there, or want to restrict them. E.g. I can
imagine that
<br>
deleting a
<br>
suffix should be prevented if it contains any
segments (or it has
<br>
to be
<br>
forced (--force option))
<br>
<br>
d) Do we want to print segments in suffix-show?
<br>
<br>
e) Mainly for Honza: I've added --show-segments
option to suffix-show
<br>
which defaults to True. I don't like the behavior
of CLI, which
<br>
asks to
<br>
confirm the value all the time. My intention was
to have it there by
<br>
default, but also allow to disable it by
--show-segments=False. I
<br>
don't
<br>
want to add it as Flag (--hide-segments) since it
restricts
<br>
versatility.
<br>
I would like to see an optional flag which would
be filled by default
<br>
value if not explicitly defined and CLI would not
ask for the
<br>
option value.
<br>
<br>
<br>
</blockquote>
<br>
Also it would be better to split the work into more
patches. E.g.
<br>
DS plugin, installation, python plugin. So ds plugin
review could
<br>
be separated from the python part.
<br>
</blockquote>
<br>
<br>
<br>
</blockquote>
<br>
<br>
<br>
</blockquote>
<br>
-- <br>
Oleg Fayans
<br>
Quality Engineer
<br>
FreeIPA team
<br>
RedHat.
<br>
</blockquote>
<br>
<br>
<br>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
</body>
</html>