<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN"> <HTML> <HEAD> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8"> <META NAME="GENERATOR" CONTENT="GtkHTML/4.6.6"> </HEAD> <BODY> On Mon, 2013-10-21 at 09:29 +0200, Martin Kosek wrote: <BLOCKQUOTE TYPE=CITE> <PRE> On 10/18/2013 05:00 PM, Martin Basti wrote: > Patch attached. > > Ticket: > <A HREF="https://fedorahosted.org/freeipa/ticket/3243">https://fedorahosted.org/freeipa/ticket/3243</A> > 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. </PRE> </BLOCKQUOTE> Separated. Patch 0014-2 -- CLI Patch 0016 -- migration <BLOCKQUOTE TYPE=CITE> <PRE> 2) When possible, please try to keep the lines up to 80 characters </PRE> </BLOCKQUOTE> <PRE> Sorry. I will to try keep length. </PRE> <BLOCKQUOTE TYPE=CITE> <PRE> 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 </PRE> </BLOCKQUOTE> <PRE> Fixed </PRE> <BLOCKQUOTE TYPE=CITE> <PRE> HTH, Martin </PRE> </BLOCKQUOTE> NOTE: Patch 0016 also changes help description of --ca-cert-file param Thank you for review and advice. <TABLE CELLSPACING="0" CELLPADDING="0" WIDTH="100%"> <TR> <TD> <PRE> -- Martin Basti </PRE> </TD> </TR> </TABLE> </BODY> </HTML>