[Freeipa-devel] [PATCH] 0014 [RFE] ipa migrate-ds should have an argument to specify cert to use for DS connection

[Martin Kosek]

On 10/18/2013 05:00 PM, Martin Basti wrote:
> Patch attached.
> 
> Ticket:
> https://fedorahosted.org/freeipa/ticket/3243
> 

I did not test the patch, just looked at the code and I have few comments:

1) Please put the ipalib/cli.py change to a sepparate change, we may want to
backport it separately some day and this will make it easier.

2) When possible, please try to keep the lines up to 80 characters

3) This part is redundant and may hide the real exception source as you are
re-raising it:

+            except Exception as e:
+                raise e


I would rather do it like that, to keep it simple:

+        cacert = None
+        if options.get('cacertfile') is not None:
+            #store CA cert into file
+            with(tempfile.NamedTemporaryFile(prefix='ipa-', suffix='.crt',
delete=False)) as tmp_ca_cert_f:
+                tmp_ca_cert_f.write(options['cacertfile'])
+                cacert = tmp_ca_cert_f.name
+
+        #start connection
+        try:
+             ds_ldap.connect(bind_dn=options['binddn'], bind_pw=bindpw,
tls_cacertfile=cacert)
+        finally:
+            #Cert file is no longer required
+            try:
+                os.remove(tmp_ca_cert_f.name)
+            except OSError:
+                pass

HTH,
Martin