On 25.01.2016 16:41, Stanislav Laznicka wrote:
Hi,

Worked those comments into the code. Also added a bit different info message in clean_ruv with ca=True (ipa-replica-manage:430).

Also adding stepst to reproduce:
1. Create a master and some replica (3 replicas is a good solution - 1 with CA, 1 without, 1 to be dangling (with CA))
2. Change domain level to 0 and ipactl restart
3. Remove the "dangling-to-be" replica from masters.ipa.etc and from both ipaca and domain subtrees in mapping tree.config
4. Try to remove the dangling ruvs with the command

Cheers,
Standa


On 01/22/2016 01:22 PM, Martin Basti wrote:
Hello,

I have a few comments

PATCH Automatically detect and remove dangling RUVs

1)
+    # get the Directory Manager password
+    if options.dirman_passwd:
+        dirman_passwd = options.dirman_passwd
+    else:
+        dirman_passwd = installutils.read_password('Directory Manager',
+            confirm=False, validate=False, retry=False)
+        if dirman_passwd is None:
+            sys.exit('Directory Manager password is required')
+
+    options.dirman_passwd = dirman_passwd

IMO you need only else branch here

if not options.dirman_password:
        dirman_passwd = installutils.read_password('Directory Manager',
            confirm=False, validate=False, retry=False)
        if dirman_passwd is None:
            sys.exit('Directory Manager password is required')
       options.dirman_passwd = dirman_passwd


2)
We should use new formatting in new code (more times in code)

+        sys.exit(
+ "Failed to get data from '%s' while trying to list replicas: %s" %
+            (host, e)
+        )

sys.exit(
"Failed to get data from '{host}' while trying to list replicas: {e}".format(
                  host=host, e=e
            )
)

3)
+        # get all masters
+        masters_dn = DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'),
+                        ipautil.realm_to_suffix(realm))

IMO you should use constants:
 masters_dn = DN(api.env.container_masters, api.env.basedn)

4)
+    # Get realm string for config tree
+    s = realm.split('.')
+    s = ['dc={dc},'.format(dc=x.lower()) for x in s]
+    realm_config = DN(('cn', ''.join(s)[0:-1]))

Can be api.env.basedn used instead of this block of code?

5)
+    masters = [x.single_value['cn'] for x in masters]
....
+    for master in masters:

is there any reason why not iterate over the keys in info dict?

for master_name, master_data/values/whatever in info.items():
   master_data['online'] = True

Looks better than: info[master]['online'] = True

6)
I asked python gurus, for empty lists and dicts, please use [] and {} instead of list() and dict()
It is preferred and faster.

7)
+            if(info[master]['ca']):
+                entry = conn.get_entry(csreplica_dn)
+ csruv = (master, entry.single_value.get('nsDS5ReplicaID'))
+                if csruv not in csruvs:
+                    csruvs.append(csruv)

I dont like too much adding tuples into list and then doing search there, but it is as designed

However can you use set() instead of list when the purpose of variable is only testing existence?

related to:
csruvs
ruvs
offlines
clean_list
cleaned

8)
conn in finally block may be undefined

9)
unused local variables

clean_list
entry on line 570

10)
optional, comment what keys means in info structure


Hello,

1)
I accept your silence as the following code cannot use nothing from api.env
+    # Get realm string for config tree
+    s = realm.split('.')
+    s = ['dc={dc},'.format(dc=x.lower()) for x in s]
+    realm_config = DN(('cn', ''.join(s)[0:-1]))

but then please use:
s = ['dc={dc}'.format(dc=x.lower()) for x in s]
realm_config = DN(('cn', ','.join(s)))

But I still think that api.env.basedn can be used, because it contains the same format as you need
realm_config = DN(('cn', api.env.basedn))

2) nitpick
ca_dn = DN(('cn', 'ca'), DN(master.dn))

AFAIK can be just

ca_dn = DN(('cn', 'ca'), master.dn)

3) uber nitpick
This is PEP8 valid, but somehow inconsistent with the rest of code and it hit my eyes

print('\t\tid: {id}, hostname: {host}'
        .format(id=csruv[1], host=csruv[0])
        )

we use in code

print(
   something1,
   something2
)

or

print(something1,
    something2)

Otherwise LGTM
-- 
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

Reply via email to