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

[Martin Basti]

On Mon, 2013-10-21 at 09:29 +0200, Martin Kosek wrote:

> 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.
> 

Separated. 
Patch 0014-2 -- CLI
Patch 0016 -- migration


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

Sorry. I will to try keep length.



> 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

Fixed



> HTH,
> Martin


NOTE: Patch 0016 also changes help description of --ca-cert-file param

Thank you for review and advice.


-- 
Martin Basti
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131021/430da0e7/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0014-2-Changed-CLI-to-allow-to-use-FILE-as-optional-param.patch
Type: text/x-patch
Size: 1130 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131021/430da0e7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0016-migrate-ds-added-ca-cert-file-FILE-option.patch
Type: text/x-patch
Size: 3073 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131021/430da0e7/attachment-0001.bin>