On 15.06.2016 15:29, Martin Babinsky wrote:
On 06/15/2016 10:30 AM, Jan Cholasta wrote:

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.


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


Attaching updated patches.

Hello, I have a few comments:

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

+ 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)
+                    )
+                )

+                    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
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

+    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:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to