On 17.06.2016 16:18, Martin Babinsky wrote:
On 06/17/2016 03:56 PM, Martin Babinsky wrote:
On 06/16/2016 12:45 PM, Martin Basti wrote:

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
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
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
in which case it should be an ipalib module.

Patch 0154: LGTM

Patch 0155:

In LDAPDelete subclasses, the primary key argument is multivalue, so
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
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,
+        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
+                                  " from s4u2proxy entry {dn}:
+ 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

Updated and rebased patches attached.

I made an error during rebase of patch 153. Re-sending the whole batch with the correct one.

ACK, rebased and pushed
* d8ae2b4055284de8c1baf76819d6611978f83cc6 ipaserver module for working with managed topology * db882ae8d6eba768e08be9317e386f8ab3c8fcf7 delegate removal of master DNS record and replica keys to separate functions * a6eb87bd68295e15ea19f5cb274cffbef5954d04 server-del: perform full master removal in managed topology
* 081941a5b9c9ac8832c465b857032e474bb9b09f CI test suite for `server-del`
* 47decc9b843b1b1d292511bcc8a24f8ac85745c0 ipa-replica-manage: use `server_del` when removing domain level 1 replica * 31ffe1a12922b5118c847cbd6ac1ca9ea232ef94 remove the master from managed topology during uninstallation

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to