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