Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 02/21/2013 12:39 PM, Jan Cholasta wrote: > On 21.2.2013 10:06, Jan Cholasta wrote: >> On 19.2.2013 17:30, Rob Crittenden wrote: >>> I think dropping raw=True in patch 99.3 is going to break a check later >>> where we look at the managedby attribute. Without raw this will be >>> managedby_host. >>> >> >> Fixed, thanks for the catch. >> >> I have also made 2 changes to patch 100 (made sure the entry returned by >> ldap2.get_ipa_config is using the correct IPASimpleLDAPObject and >> changed LDAPEntry.clone to be less fragile). >> >> Updated (and rebased) patches attached. >> >> Honza >> > > Whoops, wrong patches. Correct patches attached. > > Honza > > These patches were pushed to master as part of the big LDAP refactoring push. See https://fedorahosted.org/freeipa/ticket/2660 for details. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 21.2.2013 10:06, Jan Cholasta wrote:
On 19.2.2013 17:30, Rob Crittenden wrote:
I think dropping raw=True in patch 99.3 is going to break a check later
where we look at the managedby attribute. Without raw this will be
managedby_host.
Fixed, thanks for the catch.
I have also made 2 changes to patch 100 (made sure the entry returned by
ldap2.get_ipa_config is using the correct IPASimpleLDAPObject and
changed LDAPEntry.clone to be less fragile).
Updated (and rebased) patches attached.
Honza
Whoops, wrong patches. Correct patches attached.
Honza
--
Jan Cholasta
>From 78d3da5cc8837ae2f3be9783df6d19af2683f8fe Mon Sep 17 00:00:00 2001
From: Jan Cholasta
Date: Thu, 31 Jan 2013 11:19:13 +0100
Subject: [PATCH 1/4] Use the dn attribute of LDAPEntry to set/get DNs of
entries.
Convert all code that uses the 'dn' key of LDAPEntry for this to use the dn
attribute instead.
---
install/tools/ipa-compliance | 10 +++
install/tools/ipa-replica-install | 2 +-
ipalib/plugins/automember.py | 9 --
ipalib/plugins/baseldap.py| 58 +++
ipalib/plugins/krbtpolicy.py | 6 ++--
ipalib/plugins/permission.py | 6 ++--
ipalib/plugins/sudorule.py| 8 --
ipalib/plugins/trust.py | 2 +-
ipalib/plugins/user.py| 9 ++
ipaserver/ipaldap.py | 4 +--
ipaserver/plugins/ldap2.py| 2 --
11 files changed, 73 insertions(+), 43 deletions(-)
diff --git a/install/tools/ipa-compliance b/install/tools/ipa-compliance
index c82e415..9b34350 100644
--- a/install/tools/ipa-compliance
+++ b/install/tools/ipa-compliance
@@ -116,7 +116,7 @@ def check_compliance(tmpdir, debug=False):
hostcount = 0
# Get the hosts first
try:
-(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', ['dn'],
+(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', [],
DN(api.env.container_host, api.env.basedn),
conn.SCOPE_ONELEVEL,
size_limit = -1)
@@ -136,10 +136,10 @@ def check_compliance(tmpdir, debug=False):
available = 0
try:
(entries, truncated) = conn.find_entries('(objectclass=ipaentitlement)',
-['dn', 'userCertificate'],
-DN(api.env.container_entitlements, api.env.basedn),
-conn.SCOPE_ONELEVEL,
-size_limit = -1)
+['userCertificate'],
+DN(api.env.container_entitlements, api.env.basedn),
+conn.SCOPE_ONELEVEL,
+size_limit = -1)
for entry in entries:
(dn, attrs) = entry
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 13c3260..846122d 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -572,7 +572,7 @@ def main():
config.dirman_password)
found = False
try:
-entry = conn.find_entries(u'fqdn=%s' % host, ['dn', 'fqdn'], DN(api.env.container_host, api.env.basedn))
+entry = conn.find_entries(u'fqdn=%s' % host, ['fqdn'], DN(api.env.container_host, api.env.basedn))
print "The host %s already exists on the master server.\nYou should remove it before proceeding:" % host
print "%% ipa host-del %s" % host
found = True
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index af39f6a..520f8a0 100644
--- a/ipalib/plugins/automember.py
+++ b/ipalib/plugins/automember.py
@@ -316,10 +316,12 @@ class automember_add_condition(LDAPUpdate):
except errors.NotFound:
failed['failed'][attr].append(regex)
+entry_attrs = entry_to_dict(entry_attrs, **options)
+
# Set failed and completed to they can be harvested in the execute super
setattr(context, 'failed', failed)
setattr(context, 'completed', completed)
-setattr(context, 'entry_attrs', dict(entry_attrs))
+setattr(context, 'entry_attrs', entry_attrs)
# Make sure to returned the failed results if there is nothing to remove
if completed == 0:
@@ -406,10 +408,13 @@ class automember_remove_condition(LDAPUpdate):
else:
failed['failed'][attr].append(regex)
entry_attrs[attr] = old_entry
+
+entry_attrs = entry_to_dict(entry_attrs, **options)
+
# Set failed and completed to they can be harvested in the execute super
setattr(context, 'failed', failed)
setattr(context, 'completed', completed)
-setattr(context, 'entry_attrs', dict(entry_attrs))
+setattr(context, 'entry_attrs', entry_attrs)
# Make sure to returned the failed results if there is nothing to remove
if completed == 0:
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 44751e1..74e2384 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 19.2.2013 17:30, Rob Crittenden wrote:
I think dropping raw=True in patch 99.3 is going to break a check later
where we look at the managedby attribute. Without raw this will be
managedby_host.
Fixed, thanks for the catch.
I have also made 2 changes to patch 100 (made sure the entry returned by
ldap2.get_ipa_config is using the correct IPASimpleLDAPObject and
changed LDAPEntry.clone to be less fragile).
Updated (and rebased) patches attached.
Honza
--
Jan Cholasta
>From 78d3da5cc8837ae2f3be9783df6d19af2683f8fe Mon Sep 17 00:00:00 2001
From: Jan Cholasta
Date: Thu, 31 Jan 2013 11:19:13 +0100
Subject: [PATCH 1/4] Use the dn attribute of LDAPEntry to set/get DNs of
entries.
Convert all code that uses the 'dn' key of LDAPEntry for this to use the dn
attribute instead.
---
install/tools/ipa-compliance | 10 +++
install/tools/ipa-replica-install | 2 +-
ipalib/plugins/automember.py | 9 --
ipalib/plugins/baseldap.py| 58 +++
ipalib/plugins/krbtpolicy.py | 6 ++--
ipalib/plugins/permission.py | 6 ++--
ipalib/plugins/sudorule.py| 8 --
ipalib/plugins/trust.py | 2 +-
ipalib/plugins/user.py| 9 ++
ipaserver/ipaldap.py | 4 +--
ipaserver/plugins/ldap2.py| 2 --
11 files changed, 73 insertions(+), 43 deletions(-)
diff --git a/install/tools/ipa-compliance b/install/tools/ipa-compliance
index c82e415..9b34350 100644
--- a/install/tools/ipa-compliance
+++ b/install/tools/ipa-compliance
@@ -116,7 +116,7 @@ def check_compliance(tmpdir, debug=False):
hostcount = 0
# Get the hosts first
try:
-(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', ['dn'],
+(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', [],
DN(api.env.container_host, api.env.basedn),
conn.SCOPE_ONELEVEL,
size_limit = -1)
@@ -136,10 +136,10 @@ def check_compliance(tmpdir, debug=False):
available = 0
try:
(entries, truncated) = conn.find_entries('(objectclass=ipaentitlement)',
-['dn', 'userCertificate'],
-DN(api.env.container_entitlements, api.env.basedn),
-conn.SCOPE_ONELEVEL,
-size_limit = -1)
+['userCertificate'],
+DN(api.env.container_entitlements, api.env.basedn),
+conn.SCOPE_ONELEVEL,
+size_limit = -1)
for entry in entries:
(dn, attrs) = entry
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 13c3260..846122d 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -572,7 +572,7 @@ def main():
config.dirman_password)
found = False
try:
-entry = conn.find_entries(u'fqdn=%s' % host, ['dn', 'fqdn'], DN(api.env.container_host, api.env.basedn))
+entry = conn.find_entries(u'fqdn=%s' % host, ['fqdn'], DN(api.env.container_host, api.env.basedn))
print "The host %s already exists on the master server.\nYou should remove it before proceeding:" % host
print "%% ipa host-del %s" % host
found = True
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index af39f6a..520f8a0 100644
--- a/ipalib/plugins/automember.py
+++ b/ipalib/plugins/automember.py
@@ -316,10 +316,12 @@ class automember_add_condition(LDAPUpdate):
except errors.NotFound:
failed['failed'][attr].append(regex)
+entry_attrs = entry_to_dict(entry_attrs, **options)
+
# Set failed and completed to they can be harvested in the execute super
setattr(context, 'failed', failed)
setattr(context, 'completed', completed)
-setattr(context, 'entry_attrs', dict(entry_attrs))
+setattr(context, 'entry_attrs', entry_attrs)
# Make sure to returned the failed results if there is nothing to remove
if completed == 0:
@@ -406,10 +408,13 @@ class automember_remove_condition(LDAPUpdate):
else:
failed['failed'][attr].append(regex)
entry_attrs[attr] = old_entry
+
+entry_attrs = entry_to_dict(entry_attrs, **options)
+
# Set failed and completed to they can be harvested in the execute super
setattr(context, 'failed', failed)
setattr(context, 'completed', completed)
-setattr(context, 'entry_attrs', dict(entry_attrs))
+setattr(context, 'entry_attrs', entry_attrs)
# Make sure to returned the failed results if there is nothing to remove
if completed == 0:
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 44751e1..74e2384 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -229,6 +229,12 @@ def entry_from_entry(entry, newentry):
for e in newentry.keys():
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
Petr Viktorin wrote: On 02/06/2013 10:55 AM, Jan Cholasta wrote: On 5.2.2013 15:45, Petr Viktorin wrote: On 02/05/2013 01:38 PM, Jan Cholasta wrote: On 4.2.2013 15:49, Petr Viktorin wrote: [...] I see one of the changes is using has_key instead of `in` for a CIDict. Given that dict.has_key() is deprecated, I think a better solution would be to add __contains__ to CIDict. Is there a reason against that? I think a separate patch with this and other changes to make CIDict more like dict would be better. OK, I'll make a patch. [...] Updated patches attached. The changes look good but I can no longer apply the patches. Can you please rebase them? Sure. ACK I think dropping raw=True in patch 99.3 is going to break a check later where we look at the managedby attribute. Without raw this will be managedby_host. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 02/06/2013 10:55 AM, Jan Cholasta wrote: On 5.2.2013 15:45, Petr Viktorin wrote: On 02/05/2013 01:38 PM, Jan Cholasta wrote: On 4.2.2013 15:49, Petr Viktorin wrote: [...] I see one of the changes is using has_key instead of `in` for a CIDict. Given that dict.has_key() is deprecated, I think a better solution would be to add __contains__ to CIDict. Is there a reason against that? I think a separate patch with this and other changes to make CIDict more like dict would be better. OK, I'll make a patch. [...] Updated patches attached. The changes look good but I can no longer apply the patches. Can you please rebase them? Sure. ACK -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 5.2.2013 15:45, Petr Viktorin wrote:
On 02/05/2013 01:38 PM, Jan Cholasta wrote:
On 4.2.2013 15:49, Petr Viktorin wrote:
[...]
I see one of the changes is using has_key instead of `in` for a CIDict.
Given that dict.has_key() is deprecated, I think a better solution would
be to add __contains__ to CIDict. Is there a reason against that?
I think a separate patch with this and other changes to make CIDict more
like dict would be better.
OK, I'll make a patch.
[...]
Updated patches attached.
The changes look good but I can no longer apply the patches. Can you
please rebase them?
Sure.
--
Jan Cholasta
>From 5ca15b97a54ae5afc85bfbed36a386b79862c6e0 Mon Sep 17 00:00:00 2001
From: Jan Cholasta
Date: Thu, 31 Jan 2013 11:19:13 +0100
Subject: [PATCH 1/4] Use the dn attribute of LDAPEntry to set/get DNs of
entries.
Convert all code that uses the 'dn' key of LDAPEntry for this to use the dn
attribute instead.
---
install/tools/ipa-compliance | 10 +++
install/tools/ipa-replica-install | 2 +-
ipalib/plugins/automember.py | 9 --
ipalib/plugins/baseldap.py| 58 +++
ipalib/plugins/krbtpolicy.py | 6 ++--
ipalib/plugins/permission.py | 6 ++--
ipalib/plugins/sudorule.py| 8 --
ipalib/plugins/trust.py | 2 +-
ipalib/plugins/user.py| 9 ++
ipaserver/ipaldap.py | 4 +--
ipaserver/plugins/ldap2.py| 2 --
11 files changed, 73 insertions(+), 43 deletions(-)
diff --git a/install/tools/ipa-compliance b/install/tools/ipa-compliance
index c82e415..9b34350 100644
--- a/install/tools/ipa-compliance
+++ b/install/tools/ipa-compliance
@@ -116,7 +116,7 @@ def check_compliance(tmpdir, debug=False):
hostcount = 0
# Get the hosts first
try:
-(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', ['dn'],
+(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', [],
DN(api.env.container_host, api.env.basedn),
conn.SCOPE_ONELEVEL,
size_limit = -1)
@@ -136,10 +136,10 @@ def check_compliance(tmpdir, debug=False):
available = 0
try:
(entries, truncated) = conn.find_entries('(objectclass=ipaentitlement)',
-['dn', 'userCertificate'],
-DN(api.env.container_entitlements, api.env.basedn),
-conn.SCOPE_ONELEVEL,
-size_limit = -1)
+['userCertificate'],
+DN(api.env.container_entitlements, api.env.basedn),
+conn.SCOPE_ONELEVEL,
+size_limit = -1)
for entry in entries:
(dn, attrs) = entry
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 13c3260..846122d 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -572,7 +572,7 @@ def main():
config.dirman_password)
found = False
try:
-entry = conn.find_entries(u'fqdn=%s' % host, ['dn', 'fqdn'], DN(api.env.container_host, api.env.basedn))
+entry = conn.find_entries(u'fqdn=%s' % host, ['fqdn'], DN(api.env.container_host, api.env.basedn))
print "The host %s already exists on the master server.\nYou should remove it before proceeding:" % host
print "%% ipa host-del %s" % host
found = True
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index af39f6a..520f8a0 100644
--- a/ipalib/plugins/automember.py
+++ b/ipalib/plugins/automember.py
@@ -316,10 +316,12 @@ class automember_add_condition(LDAPUpdate):
except errors.NotFound:
failed['failed'][attr].append(regex)
+entry_attrs = entry_to_dict(entry_attrs, **options)
+
# Set failed and completed to they can be harvested in the execute super
setattr(context, 'failed', failed)
setattr(context, 'completed', completed)
-setattr(context, 'entry_attrs', dict(entry_attrs))
+setattr(context, 'entry_attrs', entry_attrs)
# Make sure to returned the failed results if there is nothing to remove
if completed == 0:
@@ -406,10 +408,13 @@ class automember_remove_condition(LDAPUpdate):
else:
failed['failed'][attr].append(regex)
entry_attrs[attr] = old_entry
+
+entry_attrs = entry_to_dict(entry_attrs, **options)
+
# Set failed and completed to they can be harvested in the execute super
setattr(context, 'failed', failed)
setattr(context, 'completed', completed)
-setattr(context, 'entry_attrs', dict(entry_attrs))
+setattr(context, 'entry_attrs', entry_attrs)
# Make sure to returned the failed results if there is nothing to remove
if completed == 0:
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 44751e1..74e2384 100644
--- a/ipalib/plug
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 02/05/2013 01:38 PM, Jan Cholasta wrote: On 4.2.2013 15:49, Petr Viktorin wrote: [...] I see one of the changes is using has_key instead of `in` for a CIDict. Given that dict.has_key() is deprecated, I think a better solution would be to add __contains__ to CIDict. Is there a reason against that? I think a separate patch with this and other changes to make CIDict more like dict would be better. OK, I'll make a patch. [...] Updated patches attached. The changes look good but I can no longer apply the patches. Can you please rebase them? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 4.2.2013 15:49, Petr Viktorin wrote:
On 02/04/2013 02:25 PM, Jan Cholasta wrote:
On 1.2.2013 12:12, Petr Viktorin wrote:
On 01/31/2013 04:18 PM, Jan Cholasta wrote:
Hi,
these patches implement attribute name case preservation in LDAPEntry.
Apply on top of Petr Viktorin's LDAP code refactoring patchset (up to
part 5).
Honza
Patches 99 & 101 need some tests to make sure the _names work correctly.
Added.
I see one of the changes is using has_key instead of `in` for a CIDict.
Given that dict.has_key() is deprecated, I think a better solution would
be to add __contains__ to CIDict. Is there a reason against that?
I think a separate patch with this and other changes to make CIDict more
like dict would be better.
Since you can call LDAPEntry.__init__ in different ways which don't
always correspond to the argument names, it would be nice to explain
them in a docstring.
Done.
A few nitpicks below.
freeipa-jcholast-99-Preserve-case-of-attribute-names-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 20c11b4..6268ac0 100644
@@ -650,14 +637,48 @@ class LDAPEntry(dict):
self._orig = self
self._orig = deepcopy(self)
+def _attr_name(self, name):
+if not isinstance(name, (unicode, str)):
Use basestring instead of (unicode, str).
Is there any compelling reason to do so? I don't want to support
arbitrary basestring subclasses in this code, just unicode and str.
Using basestring is the standard idiom. Since you're supporting
arbitrary str and unicode subclasses anyway, I don't see your point.
Besides, basestring can't even be instantiated. Someone who'd subclass
basestring directly would really have to know what they're doing.
OK. I just don't like basestring I guess. Fixed.
freeipa-jcholast-100-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 6268ac0..01fa0c1 100644
@@ -595,8 +600,10 @@ class LDAPEntry(dict):
_obj = {}
orig = None
+assert isinstance(_conn, IPASimpleLDAPObject)
assert isinstance(_dn, DN)
+self._conn = lambda: _conn # do not deepcopy me!
This would be better done by overriding __deepcopy__, or just using a
custom method for the copying.
I have tried multiple different solutions and none is as elegant as
this. Yes, it is a hack, but since it is an internal implementation
detail of LDAPEntry, I don't see any harm in doing it.
On further thought, custom method is probably better suited for this job
than than deepcopy. Added.
Updated patches attached.
Could you also add a docstring to commit()? The function is not clear
from the name alone.
Done.
Other than that, the patches look good.
Updated patches attached.
Honza
--
Jan Cholasta
>From 8778f668591e28d78741df55dc2bca98917073e5 Mon Sep 17 00:00:00 2001
From: Jan Cholasta
Date: Thu, 31 Jan 2013 11:19:13 +0100
Subject: [PATCH 1/4] Use the dn attribute of LDAPEntry to set/get DNs of
entries.
Convert all code that uses the 'dn' key of LDAPEntry for this to use the dn
attribute instead.
---
install/tools/ipa-compliance | 10 +++
install/tools/ipa-replica-install | 2 +-
ipalib/plugins/automember.py | 9 --
ipalib/plugins/baseldap.py| 58 +++
ipalib/plugins/krbtpolicy.py | 6 ++--
ipalib/plugins/permission.py | 6 ++--
ipalib/plugins/sudorule.py| 8 --
ipalib/plugins/trust.py | 2 +-
ipalib/plugins/user.py| 9 ++
ipaserver/ipaldap.py | 4 +--
ipaserver/plugins/ldap2.py| 2 --
11 files changed, 73 insertions(+), 43 deletions(-)
diff --git a/install/tools/ipa-compliance b/install/tools/ipa-compliance
index c82e415..9b34350 100644
--- a/install/tools/ipa-compliance
+++ b/install/tools/ipa-compliance
@@ -116,7 +116,7 @@ def check_compliance(tmpdir, debug=False):
hostcount = 0
# Get the hosts first
try:
-(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', ['dn'],
+(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', [],
DN(api.env.container_host, api.env.basedn),
conn.SCOPE_ONELEVEL,
size_limit = -1)
@@ -136,10 +136,10 @@ def check_compliance(tmpdir, debug=False):
available = 0
try:
(entries, truncated) = conn.find_entries('(objectclass=ipaentitlement)',
-['dn', 'userCertificate'],
-DN(api.env.container_entitlements, api.env.basedn),
-conn.SCOPE_ONELEVEL,
-size_limit = -1)
+['userCertificate'],
+DN(api.env.container_entitlements, api.env.basedn),
+conn.SCOPE_ONELEVEL,
+size_limit = -1)
for entry in entries:
(dn, attrs) = entry
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 13c3260..846122d 100755
--- a/install/tools/ipa-replica
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 02/04/2013 02:25 PM, Jan Cholasta wrote:
On 1.2.2013 12:12, Petr Viktorin wrote:
On 01/31/2013 04:18 PM, Jan Cholasta wrote:
Hi,
these patches implement attribute name case preservation in LDAPEntry.
Apply on top of Petr Viktorin's LDAP code refactoring patchset (up to
part 5).
Honza
Patches 99 & 101 need some tests to make sure the _names work correctly.
Added.
I see one of the changes is using has_key instead of `in` for a CIDict.
Given that dict.has_key() is deprecated, I think a better solution would
be to add __contains__ to CIDict. Is there a reason against that?
Since you can call LDAPEntry.__init__ in different ways which don't
always correspond to the argument names, it would be nice to explain
them in a docstring.
Done.
A few nitpicks below.
freeipa-jcholast-99-Preserve-case-of-attribute-names-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 20c11b4..6268ac0 100644
@@ -650,14 +637,48 @@ class LDAPEntry(dict):
self._orig = self
self._orig = deepcopy(self)
+def _attr_name(self, name):
+if not isinstance(name, (unicode, str)):
Use basestring instead of (unicode, str).
Is there any compelling reason to do so? I don't want to support
arbitrary basestring subclasses in this code, just unicode and str.
Using basestring is the standard idiom. Since you're supporting
arbitrary str and unicode subclasses anyway, I don't see your point.
Besides, basestring can't even be instantiated. Someone who'd subclass
basestring directly would really have to know what they're doing.
freeipa-jcholast-100-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 6268ac0..01fa0c1 100644
@@ -595,8 +600,10 @@ class LDAPEntry(dict):
_obj = {}
orig = None
+assert isinstance(_conn, IPASimpleLDAPObject)
assert isinstance(_dn, DN)
+self._conn = lambda: _conn # do not deepcopy me!
This would be better done by overriding __deepcopy__, or just using a
custom method for the copying.
I have tried multiple different solutions and none is as elegant as
this. Yes, it is a hack, but since it is an internal implementation
detail of LDAPEntry, I don't see any harm in doing it.
On further thought, custom method is probably better suited for this job
than than deepcopy. Added.
Updated patches attached.
Could you also add a docstring to commit()? The function is not clear
from the name alone.
Other than that, the patches look good.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 1.2.2013 12:12, Petr Viktorin wrote:
On 01/31/2013 04:18 PM, Jan Cholasta wrote:
Hi,
these patches implement attribute name case preservation in LDAPEntry.
Apply on top of Petr Viktorin's LDAP code refactoring patchset (up to
part 5).
Honza
Patches 99 & 101 need some tests to make sure the _names work correctly.
Added.
Since you can call LDAPEntry.__init__ in different ways which don't
always correspond to the argument names, it would be nice to explain
them in a docstring.
Done.
A few nitpicks below.
freeipa-jcholast-99-Preserve-case-of-attribute-names-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 20c11b4..6268ac0 100644
@@ -650,14 +637,48 @@ class LDAPEntry(dict):
self._orig = self
self._orig = deepcopy(self)
+def _attr_name(self, name):
+if not isinstance(name, (unicode, str)):
Use basestring instead of (unicode, str).
Is there any compelling reason to do so? I don't want to support
arbitrary basestring subclasses in this code, just unicode and str.
freeipa-jcholast-100-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 6268ac0..01fa0c1 100644
@@ -595,8 +600,10 @@ class LDAPEntry(dict):
_obj = {}
orig = None
+assert isinstance(_conn, IPASimpleLDAPObject)
assert isinstance(_dn, DN)
+self._conn = lambda: _conn # do not deepcopy me!
This would be better done by overriding __deepcopy__, or just using a
custom method for the copying.
I have tried multiple different solutions and none is as elegant as
this. Yes, it is a hack, but since it is an internal implementation
detail of LDAPEntry, I don't see any harm in doing it.
On further thought, custom method is probably better suited for this job
than than deepcopy. Added.
Updated patches attached.
Honza
--
Jan Cholasta
>From 8778f668591e28d78741df55dc2bca98917073e5 Mon Sep 17 00:00:00 2001
From: Jan Cholasta
Date: Thu, 31 Jan 2013 11:19:13 +0100
Subject: [PATCH 1/4] Use the dn attribute of LDAPEntry to set/get DNs of
entries.
Convert all code that uses the 'dn' key of LDAPEntry for this to use the dn
attribute instead.
---
install/tools/ipa-compliance | 10 +++
install/tools/ipa-replica-install | 2 +-
ipalib/plugins/automember.py | 9 --
ipalib/plugins/baseldap.py| 58 +++
ipalib/plugins/krbtpolicy.py | 6 ++--
ipalib/plugins/permission.py | 6 ++--
ipalib/plugins/sudorule.py| 8 --
ipalib/plugins/trust.py | 2 +-
ipalib/plugins/user.py| 9 ++
ipaserver/ipaldap.py | 4 +--
ipaserver/plugins/ldap2.py| 2 --
11 files changed, 73 insertions(+), 43 deletions(-)
diff --git a/install/tools/ipa-compliance b/install/tools/ipa-compliance
index c82e415..9b34350 100644
--- a/install/tools/ipa-compliance
+++ b/install/tools/ipa-compliance
@@ -116,7 +116,7 @@ def check_compliance(tmpdir, debug=False):
hostcount = 0
# Get the hosts first
try:
-(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', ['dn'],
+(entries, truncated) = conn.find_entries('(krblastpwdchange=*)', [],
DN(api.env.container_host, api.env.basedn),
conn.SCOPE_ONELEVEL,
size_limit = -1)
@@ -136,10 +136,10 @@ def check_compliance(tmpdir, debug=False):
available = 0
try:
(entries, truncated) = conn.find_entries('(objectclass=ipaentitlement)',
-['dn', 'userCertificate'],
-DN(api.env.container_entitlements, api.env.basedn),
-conn.SCOPE_ONELEVEL,
-size_limit = -1)
+['userCertificate'],
+DN(api.env.container_entitlements, api.env.basedn),
+conn.SCOPE_ONELEVEL,
+size_limit = -1)
for entry in entries:
(dn, attrs) = entry
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 13c3260..846122d 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -572,7 +572,7 @@ def main():
config.dirman_password)
found = False
try:
-entry = conn.find_entries(u'fqdn=%s' % host, ['dn', 'fqdn'], DN(api.env.container_host, api.env.basedn))
+entry = conn.find_entries(u'fqdn=%s' % host, ['fqdn'], DN(api.env.container_host, api.env.basedn))
print "The host %s already exists on the master server.\nYou should remove it before proceeding:" % host
print "%% ipa host-del %s" % host
found = True
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index af39f6a..520f8a0 100644
--- a/ipalib/plugins/automember.py
+++ b/ipalib/plugins/automember.py
@@ -316,10 +316,12 @@ class automember_add_condition(LDAPUpdate):
except errors.NotFound:
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 01/31/2013 04:18 PM, Jan Cholasta wrote:
Hi,
these patches implement attribute name case preservation in LDAPEntry.
Apply on top of Petr Viktorin's LDAP code refactoring patchset (up to
part 5).
Honza
Patches 99 & 101 need some tests to make sure the _names work correctly.
Since you can call LDAPEntry.__init__ in different ways which don't
always correspond to the argument names, it would be nice to explain
them in a docstring.
A few nitpicks below.
freeipa-jcholast-99-Preserve-case-of-attribute-names-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 20c11b4..6268ac0 100644
@@ -650,14 +637,48 @@ class LDAPEntry(dict):
self._orig = self
self._orig = deepcopy(self)
+def _attr_name(self, name):
+if not isinstance(name, (unicode, str)):
Use basestring instead of (unicode, str).
freeipa-jcholast-100-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 6268ac0..01fa0c1 100644
@@ -595,8 +600,10 @@ class LDAPEntry(dict):
_obj = {}
orig = None
+assert isinstance(_conn, IPASimpleLDAPObject)
assert isinstance(_dn, DN)
+self._conn = lambda: _conn # do not deepcopy me!
This would be better done by overriding __deepcopy__, or just using a
custom method for the copying.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
