On 6.6.2016 15:25, Fraser Tweedale wrote:
On Wed, Jun 01, 2016 at 02:49:06PM +1000, Fraser Tweedale wrote:
Updated patch attached; comments inline below.

On Mon, Apr 25, 2016 at 07:55:46AM +0200, Jan Cholasta wrote:
I think it would be better to merge the `client` and `client_servicename`
into a single `client_principal` argument, as both of the arguments are used
only to specify the principal name of the client.

Done.

Also I would prefer if the keyfile and keytab arguments were required,
because it's better if you can explicitly see what values are used at the
call site.

Done.

Why is init_creds() now called from __init__()? Why is it still called from
_auth_header()?

I invoke it from __init__ for more eager failure if there's a
problem (e.g. service is not in keytab), giving better error
locality.

It remains necessary to invoke it from _auth_header in case of
short-lived credentials.  I added some comments to the source.

Why is ldap_uri now passed to IPAKEMKeys()?

It tries to use LDAPI by default, so ldap_uri needs to be passed
through if process owner cannot access LDAPI socket.  I added
commentary to source.

Regarding your suggestion to make a base class and override class
variables, I prefer to pass values around.  There are very few
instantiations of CustodiaClient so it is easy enough to follow.

It may be easy to follow, but what you are currently doing is basically emulating subclassing with functools.partial, which is both unidiomatic and less readable than actual subclassing.

But we are past the nitpicking phase, so I'm going to let that slide :-)


Self-NACK on 0051-4; rebased and updated patch attached.

Only one substantive change, in custodiainstance.py:

-            client_service='host@' + self.fqdn,
+            client_service='host@%s' % self.fqdn,

First version broke uninstall because CustodiaInstance gets
initialised with 'host_name = None' and '+' doesn't like None.

This will give "host@None" on uninstall, but I guess that's OK, since it's currently not used during uninstall.

ACK.

Pushed to master: f94ccca6761f7dbe3f99855d181fe2cec380d476

--
Jan Cholasta

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