On 24.11.2015 10:15, Martin Basti wrote:



On 20.11.2015 09:00, Jan Cholasta wrote:
On 19.11.2015 14:13, Jan Cholasta wrote:
On 19.11.2015 14:09, Martin Babinsky wrote:
On 11/19/2015 01:08 PM, Martin Basti wrote:


On 18.11.2015 14:26, Martin Basti wrote:


On 18.11.2015 14:24, Martin Kosek wrote:
On 11/18/2015 02:18 PM, Martin Basti wrote:

On 18.11.2015 13:55, Martin Kosek wrote:
On 11/18/2015 01:30 PM, Martin Basti wrote:
+    DEFAULT_TIME_LIMIT = -1.0
+    DEFAULT_SIZE_LIMIT = 0
...
           if time_limit is None or time_limit == 0:
-            time_limit = -1.0
+            if self.time_limit is not None and self.time_limit
!= 0:
+                time_limit = self.time_limit
+            else:
+                time_limit = self.DEFAULT_TIME_LIMIT
+
I wonder why is the -1 default time limit a float number, I would
expect that
some trouble may emerge out of it. Maybe Rob knows?

Python LDAP allows to have smaller granularity than seconds and it
internally
convert time limit values to float.
Hm, ok. I was just curious since the value we expose in API is Int
and the
default does not use the smaller granularity, so I was thinking "Why
bother?".
If it is fixed in your patch, good. If not, good also, no need to
bikeshed here
I suppose.
I just keep the original values there, I do not want to touch it with
this patch

Updated patch attached, the original one has wrong default limit for
ldap2


ACK

Hold your horses.

1) The upgrade_ldapsearch_limit option is not very useful, either add
generic search_time_limit and search_records_limit options, or don't
add any options at all (which, incidentally, is exactly what the
ticket asks for).

2) The limits are not passed to the connection using __init__()
anywhere, so there is no need to have arguments for them.

3) Rather than adding layers of if statements everywhere, implement
the time_limit and size_limit attributes as properties.

4) Use config_entry.single_value when setting defaults in
ldap2.get_ipa_config().

BTW, add comments only when it's not obvious what the code does - a
comment saying "set defaults" above two .setdefault() calls is pretty
redundant.

Updated patch attached

In ldap2.create_connection(), you should set self.time_limit only if time_limit is not None, ditto for size limit, instead of handling None in the properties' setters.

You don't need to handle the limits in ldap2.find_entries() anymore, since it is handled by LDAPClient.find_entries(), so you can remove the related code completely from ldap2.find_entries().

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