Pylint: remove unused variables #135
Conversation
MartinBasti
changed the title
There was a problem hiding this comment.
I saw omitting the truncation flag many times during the review. Please check the marked spots and see if it would be reasonable adding size_limit=-1.
I marked the spots only in one patch, my marking got lost in the other, unfortunately, but the other seemed fine except this needs checking as well.
I am not sure how big of an issue can a low size_limit get so if you don't agree it's worth it then the patch would be fine. Also note that this issue would appear only in ldap2 connections, LDAPClient connections should be fine.
| (entries, truncated) = api.Backend.ldap2.find_entries(filter=filter, | ||
| base_dn=base_dn, attrs_list=['']) | ||
| entries, _truncated = api.Backend.ldap2.find_entries( | ||
| filter=filter, base_dn=base_dn, attrs_list=['']) |
There was a problem hiding this comment.
As truncated is ignored here, could you please also add size_limit=-1 to arguments of find_entries so that the below comment is always true?
| base_dn=agents_dn, attrs_list=['member'], scope=ldap.SCOPE_BASE) | ||
| entries_a, _truncated = smb.admin_conn.find_entries( | ||
| filter="", base_dn=agents_dn, attrs_list=['member'], | ||
| scope=ldap.SCOPE_BASE) |
There was a problem hiding this comment.
Adding size_limit=-1 here is probably not necessary as ldap2 is not used here but feel free to if you feel like it.
| @@ -45,7 +42,7 @@ def _find_dnssec_enabled_zones(conn): | |||
| dnssec_enabled_filter = conn.make_filter(search_kw) | |||
| dn = DN('cn=dns', api.env.basedn) | |||
| try: | |||
| entries, truncated = conn.find_entries( | |||
| entries, _truncated = conn.find_entries( | |||
| base_dn=dn, filter=dnssec_enabled_filter, attrs_list=['idnsname']) | |||
There was a problem hiding this comment.
We may need the size limit here as well.
| @@ -117,7 +114,7 @@ def remove_replica_public_keys(self, replica_fqdn): | |||
| 'ipk11Wrap': True, | |||
| } | |||
| filter = ldap.make_filter(search_kw, rules=ldap.MATCH_ALL) | |||
| entries, truncated = ldap.find_entries(filter=filter, base_dn=dn_base) | |||
| entries, _truncated = ldap.find_entries(filter=filter, base_dn=dn_base) | |||
| @@ -394,7 +387,7 @@ def __setup_replica_keys(self): | |||
| 'ipk11Wrap': True, | |||
| } | |||
| filter = ldap.make_filter(search_kw, rules=ldap.MATCH_ALL) | |||
| entries, truncated = ldap.find_entries(filter=filter, | |||
| entries, _truncated = ldap.find_entries(filter=filter, | |||
| base_dn=dn_base) | |||
| @@ -193,7 +191,7 @@ def find_winsync_users(self): | |||
|
|
|||
| user_filter = "(&(objectclass=ntuser)(ntUserDomainId=*))" | |||
| user_base = DN(api.env.container_user, api.env.basedn) | |||
| entries, _ = self.ldap.find_entries( | |||
| entries, _truncated = self.ldap.find_entries( | |||
| filter=user_filter, | |||
| base_dn=user_base, | |||
| paged_search=True) | |||
There was a problem hiding this comment.
paged_search=True should suffice I think.
| base_dn=object_container_dn) | ||
| objects, _truncated = self.ldap.find_entries( | ||
| member_filter, | ||
| base_dn=object_container_dn) |
| update_list = [] | ||
| restart = False | ||
|
|
||
| # If the old entries don't exist the server has already been updated. | ||
| try: | ||
| definitions_managed_entries, truncated = ldap.find_entries( | ||
| definitions_managed_entries, _truncated = ldap.find_entries( | ||
| searchfilter, ['*'], old_definition_container, | ||
| ldap.SCOPE_ONELEVEL) |
There was a problem hiding this comment.
None of above is aim of this commit, feel free to open ticket
|
A refactoring ticket needs opening for the issues with find_entries mentioned here. Tests seem to pass, so ACK. |
Would be nice to merge this patch before refactoring of installers starts