<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 15.06.2016 15:29, Martin Babinsky
wrote:<br>
</div>
<blockquote
cite="mid:07067481-f228-4a92-64a3-7569bf66c7cf@redhat.com"
type="cite">On 06/15/2016 10:30 AM, Jan Cholasta wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
On 12.6.2016 17:31, Martin Babinsky wrote:
<br>
<blockquote type="cite">On 06/09/2016 08:12 PM, Martin Babinsky
wrote:
<br>
<blockquote type="cite">These patches expand `server_del` to a
full fledged IPA master killer in
<br>
domain level 1.
<br>
<br>
Due to 'server uninstallation removed master from topology'
use case,
<br>
the individual steps are not in the same order as in the
original code
<br>
to facilitate self-removal from topology without introducing
an array of
<br>
permissions for master to remove itself.
<br>
<br>
I had no opportunity to test out the CI test suite because
of technical
<br>
problems so it would be nice if our upstream QE could give
it a spin and
<br>
report errors.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://www.freeipa.org/page/V4/Manage_replication_topology_4_4">http://www.freeipa.org/page/V4/Manage_replication_topology_4_4</a>
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5181">https://fedorahosted.org/freeipa/ticket/5181</a>
<br>
<br>
<br>
<br>
</blockquote>
Attaching rebased patches and bumping for review.
<br>
<br>
Please note that they depend on 'Server Roles v2' patchset.
<br>
</blockquote>
<br>
Patch 0153:
<br>
<br>
Should be an ipaserver module, unless it is required on clients
as well,
<br>
in which case it should be an ipalib module.
<br>
<br>
<br>
Patch 0154: LGTM
<br>
<br>
<br>
Patch 0155:
<br>
<br>
In LDAPDelete subclasses, the primary key argument is
multivalue, so I'm
<br>
guessing your post_callback won't work correctly.
<br>
<br>
Also, since this is *server*-del, s/master/server/ where
applicable.
<br>
<br>
<br>
Patch 0156: LGTM
<br>
<br>
<br>
Patch 0157:
<br>
<br>
This looks suspicious:
<br>
<br>
+ result = server_del_cmd(hostname, version=api_version,
**options)
<br>
<br>
Version is automatically filled in in Command.__call__(), why do
you add
<br>
it manually here?
<br>
<br>
<br>
Patch 0158: LGTM
<br>
<br>
<br>
Honza
<br>
<br>
</blockquote>
<br>
Attaching updated patches.
<br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
Hello, I have a few comments:<br>
<br>
1)<br>
you reused ID numbers for the your messages<br>
<br>
+class ServerRemovalInfo(PublicMessage):<br>
...<br>
+ errno = 13020<br>
<br>
+class ServerRemovalWarning(PublicMessage):<br>
...<br>
+ errno = 13021<br>
<br>
<br>
class FailedToRemoveHostDNSRecords(PublicMessage):<br>
... <br>
errno = 13020<br>
<br>
<br>
class DNSForwardPolicyConflictWithEmptyZone(PublicMessage):<br>
... <br>
errno = 13021<br>
<br>
2)<br>
+ def _check_topology_connectivity(self, topology_connectivity,
master_cn):<br>
+ try:<br>
+ topology_connectivity.check_current_state()<br>
+ except ValueError as e:<br>
+ raise errors.ServerRemovalError(reason=_(str(e)))<br>
+<br>
+ try:<br>
+
topology_connectivity.check_state_after_removal(master_cn)<br>
+ except ValueError as e:<br>
+ raise errors.ServerRemovalError(reason=_(str(e)))<br>
<br>
* _(str(e)): gettext cannot be used by this way<br>
* str(e): you dont need to convert exception to string, this is done
automatically in exception<br>
<br>
<br>
3) gettext again<br>
+ self.add_message(<br>
+ messages.ServerRemovalWarning(<br>
+ message=_(msg)<br>
+ )<br>
+ )<br>
<br>
<br>
4)<br>
+ messages.ServerRemovalWarning(<br>
+ message=_("Failed to clean memberPrincipal
{principal}"<br>
+ " from s4u2proxy entry {dn}:
{err}".format(<br>
+ principal=member_principal,<br>
+ dn=dn,<br>
+ err=e))))<br>
<br>
+ messages.ServerRemovalWarning(<br>
+ message=_("Failed to clean up DNA hostname
entries for "<br>
+ "{master}: {err}".format(<br>
+ master=master, err=e))))<br>
<br>
several more times<br>
<br>
I'm not sure if this will work, for safety I would prefer to change
it to dictionary substitution<br>
<a class="moz-txt-link-freetext" href="https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=668226">https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=668226</a><br>
It looks like it was fixed in gettext 18.3, some distributions still
have the older one<br>
<br>
I have to test more how gettext works with the new python format
strings<br>
<br>
5)<br>
+ def interactive_prompt_callback(self, kw):<br>
+ self.api.Backend.textui.print_plain(<br>
+ _("Removing {server} from replication topology, "<br>
+ "please wait...".format(server=', '.join(kw['cn']))))<br>
<br>
Will this work? IMO this should be on client side<br>
<br>
<br>
</body>
</html>