[Freeipa-devel] [PATCH 0153-0158] move ipa-replica-manage del functionality into server-del

[Martin Basti]

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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160616/4625b82a/attachment.htm>