Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 02/27/2013 01:59 PM, Jan Cholasta wrote: On 26.2.2013 11:03, Petr Viktorin wrote: Thanks. I think you should also add a tearDown method to test_LDAPEntry which disconnects self.conn if it is connected (the same thing test_ldap does). Thanks for the catch, added. ACK. 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] 146-164 LDAP code refactoring (Part 4)
On 26.2.2013 11:03, Petr Viktorin wrote: Thanks. I think you should also add a tearDown method to test_LDAPEntry which disconnects self.conn if it is connected (the same thing test_ldap does). Thanks for the catch, added. ACK. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 20.2.2013 13:03, Petr Viktorin wrote: On 02/19/2013 03:10 PM, Jan Cholasta wrote: On 1.2.2013 15:38, Petr Viktorin wrote: Alright, I renamed get_single to single_value(). I also rebased to current master. Patch 152: +def single_value(self, name, default=_missing): +values = self.get(name, [default]) +if len(values) != 1: +raise ValueError( +'%s has %s values, one expected' % (name, len(values))) +if values[0] is _missing: +raise KeyError(name) +return values[0] I would prefer if you used __getitem__() instead of get() and re-raised KeyError if default is _missing. Also, until we disallow non-lists, we need to check if values actually is list. I.e., do something like this: +def single_value(self, name, default=_missing): +try: +values = super(LDAPEntry, self).__getitem__(self._get_attr_name(name)) +except KeyError: +if default is _missing: +raise +return default +if not isinstance(values, list): +return values +if len(values) != 1: +raise ValueError( +'%s has %s values, one expected' % (name, len(values))) +return values[0] Updated, thanks. _get_attr_name is only added in your patch 99, so I used _attr_name here and modified your patch. I wrote some tests for single_value, and while I was at it I added tests for a few other LDAPEntry methods as well. I've also split things up into more testcases. Including as patch 0181. Thanks. I think you should also add a tearDown method to test_LDAPEntry which disconnects self.conn if it is connected (the same thing test_ldap does). Patch 159: Like I said in my review of your patch 143, I think we should use DNs instead of entries in delete_entry, so I think it would make sense to do delete_entry(entry.dn) in this patch. I think that for symmetry with add_entry and update_entry, delete_entry should take entries too. If you already have the entry, having to type the extra .dn is not very intuitive. The proper thing to do would be a separate delete_by_dn(), but delete_entry() taking either entries or DNs works fine IMO. OK, makes sense. Patch 160: I think you should do this (replace % with ,): +root_logger.debug( +failed to find mapping tree entry for %s, self.suffix) Fixed, thanks. I've also rebased 174 and your patch 104 to current master. Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 1.2.2013 15:38, Petr Viktorin wrote: Alright, I renamed get_single to single_value(). I also rebased to current master. Patch 152: +def single_value(self, name, default=_missing): +values = self.get(name, [default]) +if len(values) != 1: +raise ValueError( +'%s has %s values, one expected' % (name, len(values))) +if values[0] is _missing: +raise KeyError(name) +return values[0] I would prefer if you used __getitem__() instead of get() and re-raised KeyError if default is _missing. Also, until we disallow non-lists, we need to check if values actually is list. I.e., do something like this: +def single_value(self, name, default=_missing): +try: +values = super(LDAPEntry, self).__getitem__(self._get_attr_name(name)) +except KeyError: +if default is _missing: +raise +return default +if not isinstance(values, list): +return values +if len(values) != 1: +raise ValueError( +'%s has %s values, one expected' % (name, len(values))) +return values[0] Patch 159: Like I said in my review of your patch 143, I think we should use DNs instead of entries in delete_entry, so I think it would make sense to do delete_entry(entry.dn) in this patch. Patch 160: I think you should do this (replace % with ,): +root_logger.debug( +failed to find mapping tree entry for %s, self.suffix) Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 02/11/2013 02:07 PM, Petr Viktorin wrote:
On 02/01/2013 03:38 PM, Petr Viktorin wrote:
On 02/01/2013 10:24 AM, Jan Cholasta wrote:
On 1.2.2013 09:47, Petr Viktorin wrote:
On 01/31/2013 07:01 PM, Jan Cholasta wrote:
On 31.1.2013 11:00, Petr Viktorin wrote:
On 01/30/2013 10:53 AM, Petr Viktorin wrote:
On 01/29/2013 04:39 PM, Petr Viktorin wrote:
On 01/28/2013 04:09 PM, Petr Viktorin wrote:
On 01/28/2013 09:34 AM, Jan Cholasta wrote:
On 25.1.2013 14:54, Petr Viktorin wrote:
On 01/24/2013 03:06 PM, Petr Viktorin wrote:
On 01/24/2013 10:43 AM, Petr Viktorin wrote:
On 01/22/2013 04:04 PM, Petr Viktorin wrote:
On 01/21/2013 06:38 PM, Petr Viktorin wrote:
On 01/17/2013 06:27 PM, Petr Viktorin wrote:
Hello,
This is the first batch of changes aimed to consolidate our
LDAP
code.
Each should be a self-contained change that doesn't break
anything.
[...]
Since this patchset is becoming unwieldy, I've put it in a
public
repo
that I'll keep updated. The following command will fetch it into
your
pviktori-ldap-refactor branch:
git fetch git://github.com/encukou/freeipa
ldap-refactor:pviktori-ldap-refactor
[...]
I would prefer if you used the semantics of .get() for
.get_single() as
well (i.e. when no default value is provided, None is assumed) in
patch
152. Or is there a reason not to?
I think you should always have to write extra code to supress
exceptions
(“Errors should never pass silently”). In Python, the easiest/shortest
getter you can write will typically raise an exception when the
value is
missing (e.g. `d[k]` for dicts, `getattr(a, b)` for objects).
That is true, but I think consistency is more important here - the name
suggests the method behaves the same way .get() does. If you insist on
keeping the current behavior, would you at least consider renaming the
method (perhaps to just single or single_value)?
(This is just a nitpick, so don't worry too much about it.)
Alright, I renamed get_single to single_value().
I also rebased to current master.
Updating patch 115 to take recent changes in replica-manage into account.
Patches 161 167 needed a rebase.
--
Petr³
From 80cccf74af15207fd6eed5e3e46e81350a3b4201 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Wed, 23 Jan 2013 10:05:21 -0500
Subject: [PATCH] replace getEntry with get_entry (or get_entries if scope !=
SCOPE_BASE)
Part of the work for: https://fedorahosted.org/freeipa/ticket/2660
---
install/tools/ipa-csreplica-manage |6 ++--
install/tools/ipa-managed-entries|7 +
install/tools/ipa-replica-manage |8 +++---
ipaserver/install/adtrustinstance.py | 25 +---
ipaserver/install/dsinstance.py |2 +-
ipaserver/install/krbinstance.py |8 --
ipaserver/install/ldapupdate.py |2 +-
ipaserver/install/replication.py | 41 +++---
ipaserver/install/service.py |2 +-
ipaserver/ipaldap.py | 21 -
10 files changed, 56 insertions(+), 66 deletions(-)
diff --git a/install/tools/ipa-csreplica-manage b/install/tools/ipa-csreplica-manage
index 5cab8b8642c4fd9113c6eb4a4aeab1b4b502cfb5..3f10c9003482c6f3903f70fd9eb59b8c469b8f42 100755
--- a/install/tools/ipa-csreplica-manage
+++ b/install/tools/ipa-csreplica-manage
@@ -135,7 +135,7 @@ class CSReplicationManager(replication.ReplicationManager):
try:
cn=%sAgreement1-%s-%s % (master, host, instance_name)
dn = DN(('cn', cn), self.replica_dn())
-self.conn.getEntry(dn, ldap.SCOPE_BASE)
+self.conn.get_entry(dn)
return (cn, dn)
except errors.NotFound:
dn = None
@@ -156,7 +156,7 @@ class CSReplicationManager(replication.ReplicationManager):
def has_ipaca(self):
try:
-entry = self.conn.getEntry(self.suffix, ldap.SCOPE_BASE)
+entry = self.conn.get_entry(self.suffix)
except errors.NotFound:
return False
else:
@@ -216,7 +216,7 @@ def list_replicas(realm, host, replica, dirman_passwd, verbose):
for ent in entries:
try:
cadn = DN(('cn', 'CA'), DN(ent.dn))
-entry = conn.getEntry(cadn, ldap.SCOPE_BASE)
+entry = conn.get_entry(cadn)
peers[ent.single_value('cn')] = ['master', '']
except errors.NotFound:
peers[ent.single_value('cn')] = ['CA not configured', '']
diff --git a/install/tools/ipa-managed-entries b/install/tools/ipa-managed-entries
index 11deb63c32ff59a36276bbee063fb55740088343..b7dbdb9d3ed9930bd3eec35bbc117e061bc26300 100755
--- a/install/tools/ipa-managed-entries
+++ b/install/tools/ipa-managed-entries
@@ -143,11 +143,8 @@ def main():
disabled = True
try:
-entry = conn.getEntry(def_dn,
-ldap.SCOPE_BASE,
-
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 01/31/2013 07:01 PM, Jan Cholasta wrote: On 31.1.2013 11:00, Petr Viktorin wrote: On 01/30/2013 10:53 AM, Petr Viktorin wrote: On 01/29/2013 04:39 PM, Petr Viktorin wrote: On 01/28/2013 04:09 PM, Petr Viktorin wrote: On 01/28/2013 09:34 AM, Jan Cholasta wrote: On 25.1.2013 14:54, Petr Viktorin wrote: On 01/24/2013 03:06 PM, Petr Viktorin wrote: On 01/24/2013 10:43 AM, Petr Viktorin wrote: On 01/22/2013 04:04 PM, Petr Viktorin wrote: On 01/21/2013 06:38 PM, Petr Viktorin wrote: On 01/17/2013 06:27 PM, Petr Viktorin wrote: Hello, This is the first batch of changes aimed to consolidate our LDAP code. Each should be a self-contained change that doesn't break anything. [...] Since this patchset is becoming unwieldy, I've put it in a public repo that I'll keep updated. The following command will fetch it into your pviktori-ldap-refactor branch: git fetch git://github.com/encukou/freeipa ldap-refactor:pviktori-ldap-refactor [...] I found a bug in patch 143, here is a fixed version. I would prefer if you used the semantics of .get() for .get_single() as well (i.e. when no default value is provided, None is assumed) in patch 152. Or is there a reason not to? I think you should always have to write extra code to supress exceptions (“Errors should never pass silently”). In Python, the easiest/shortest getter you can write will typically raise an exception when the value is missing (e.g. `d[k]` for dicts, `getattr(a, b)` for objects). -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 1.2.2013 09:47, Petr Viktorin wrote: On 01/31/2013 07:01 PM, Jan Cholasta wrote: On 31.1.2013 11:00, Petr Viktorin wrote: On 01/30/2013 10:53 AM, Petr Viktorin wrote: On 01/29/2013 04:39 PM, Petr Viktorin wrote: On 01/28/2013 04:09 PM, Petr Viktorin wrote: On 01/28/2013 09:34 AM, Jan Cholasta wrote: On 25.1.2013 14:54, Petr Viktorin wrote: On 01/24/2013 03:06 PM, Petr Viktorin wrote: On 01/24/2013 10:43 AM, Petr Viktorin wrote: On 01/22/2013 04:04 PM, Petr Viktorin wrote: On 01/21/2013 06:38 PM, Petr Viktorin wrote: On 01/17/2013 06:27 PM, Petr Viktorin wrote: Hello, This is the first batch of changes aimed to consolidate our LDAP code. Each should be a self-contained change that doesn't break anything. [...] Since this patchset is becoming unwieldy, I've put it in a public repo that I'll keep updated. The following command will fetch it into your pviktori-ldap-refactor branch: git fetch git://github.com/encukou/freeipa ldap-refactor:pviktori-ldap-refactor [...] I found a bug in patch 143, here is a fixed version. I would prefer if you used the semantics of .get() for .get_single() as well (i.e. when no default value is provided, None is assumed) in patch 152. Or is there a reason not to? I think you should always have to write extra code to supress exceptions (“Errors should never pass silently”). In Python, the easiest/shortest getter you can write will typically raise an exception when the value is missing (e.g. `d[k]` for dicts, `getattr(a, b)` for objects). That is true, but I think consistency is more important here - the name suggests the method behaves the same way .get() does. If you insist on keeping the current behavior, would you at least consider renaming the method (perhaps to just single or single_value)? (This is just a nitpick, so don't worry too much about it.) Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 01/30/2013 10:53 AM, Petr Viktorin wrote:
On 01/29/2013 04:39 PM, Petr Viktorin wrote:
On 01/28/2013 04:09 PM, Petr Viktorin wrote:
On 01/28/2013 09:34 AM, Jan Cholasta wrote:
On 25.1.2013 14:54, Petr Viktorin wrote:
On 01/24/2013 03:06 PM, Petr Viktorin wrote:
On 01/24/2013 10:43 AM, Petr Viktorin wrote:
On 01/22/2013 04:04 PM, Petr Viktorin wrote:
On 01/21/2013 06:38 PM, Petr Viktorin wrote:
On 01/17/2013 06:27 PM, Petr Viktorin wrote:
Hello,
This is the first batch of changes aimed to consolidate our LDAP
code.
Each should be a self-contained change that doesn't break
anything.
[...]
Since this patchset is becoming unwieldy, I've put it in a public repo
that I'll keep updated. The following command will fetch it into your
pviktori-ldap-refactor branch:
git fetch git://github.com/encukou/freeipa
ldap-refactor:pviktori-ldap-refactor
[...]
I found a bug in patch 143, here is a fixed version.
--
Petr³
From 383f19456dd695a2132e0cf0dab244237b964ec3 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Wed, 23 Jan 2013 06:38:32 -0500
Subject: [PATCH] Change {add,update,delete}_entry to take LDAPEntries
These methods currently take (dn, entry_attrs, normalize=True)
(or (dn, normalize=True) for delete).
Change them to also accept just an LDAPEntry, and document the
legacy calling style as such.
Part of the work for: https://fedorahosted.org/freeipa/ticket/2660
---
ipaserver/ipaldap.py | 83 +-
1 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 27016e92f9435461aedee98ecb82482913d0e435..6d92a11b590ef05454f99acc81766951cf38e347 100644
--- a/ipaserver/ipaldap.py
+++ b/ipaserver/ipaldap.py
@@ -1354,21 +1354,40 @@ class LDAPConnection(object):
self.log.debug(get_members: result=%s, entries)
return entries
-def add_entry(self, dn, entry_attrs, normalize=True):
-Create a new entry.
-
-assert isinstance(dn, DN)
-
-if normalize:
-dn = self.normalize_dn(dn)
-# remove all None or [] values, python-ldap hates'em
-entry_attrs = dict(
-# FIXME, shouldn't these values be an error?
-(k, v) for (k, v) in entry_attrs.iteritems()
-if v is not None and v != []
-)
+def _get_dn_and_attrs(self, entry_or_dn, entry_attrs, normalize):
+Helper for legacy calling style for {add,update}_entry
+
+if entry_attrs is None:
+assert normalize is None
+return entry_or_dn.dn, entry_or_dn
+else:
+assert isinstance(entry_or_dn, DN)
+if normalize is None or normalize:
+entry_or_dn = self.normalize_dn(entry_or_dn)
+entry_attrs = dict(entry_attrs)
+for key, value in entry_attrs.items():
+if value is None:
+entry_attrs[key] = []
+return entry_or_dn, entry_attrs
+
+def add_entry(self, entry, entry_attrs=None, normalize=None):
+Create a new entry.
+
+This should be called as add_entry(entry).
+
+The legacy two/three-argument variant is:
+add_entry(dn, entry_attrs, normalize=True)
+
+dn, attrs = self._get_dn_and_attrs(entry, entry_attrs, normalize)
+
+# remove all [] values (python-ldap hates 'em)
+attrs = dict((k, v) for k, v in attrs.iteritems()
+# FIXME: Once entry values are always lists, this condition can
+# be just if v:
+if v is not None and v != [])
+
try:
-self.conn.add_s(dn, list(entry_attrs.iteritems()))
+self.conn.add_s(dn, list(attrs.iteritems()))
except _ldap.LDAPError, e:
self.handle_errors(e)
@@ -1455,34 +1474,42 @@ class LDAPConnection(object):
return modlist
-def update_entry(self, dn, entry_attrs, normalize=True):
-
-Update entry's attributes.
+def update_entry(self, entry, entry_attrs=None, normalize=None):
+Update entry's attributes.
-An attribute value set to None deletes all current values.
-
+This should be called as update_entry(entry).
-assert isinstance(dn, DN)
-if normalize:
-dn = self.normalize_dn(dn)
+The legacy two/three-argument variant is:
+update_entry(dn, entry_attrs, normalize=True)
+
+dn, attrs = self._get_dn_and_attrs(entry, entry_attrs, normalize)
# generate modlist
-modlist = self._generate_modlist(dn, entry_attrs, normalize)
+modlist = self._generate_modlist(dn, attrs, normalize)
if not modlist:
raise errors.EmptyModlist()
# pass arguments to python-ldap
try:
self.conn.modify_s(dn, modlist)
except _ldap.LDAPError, e:
self.handle_errors(e)
-def delete_entry(self,
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 31.1.2013 11:00, Petr Viktorin wrote: On 01/30/2013 10:53 AM, Petr Viktorin wrote: On 01/29/2013 04:39 PM, Petr Viktorin wrote: On 01/28/2013 04:09 PM, Petr Viktorin wrote: On 01/28/2013 09:34 AM, Jan Cholasta wrote: On 25.1.2013 14:54, Petr Viktorin wrote: On 01/24/2013 03:06 PM, Petr Viktorin wrote: On 01/24/2013 10:43 AM, Petr Viktorin wrote: On 01/22/2013 04:04 PM, Petr Viktorin wrote: On 01/21/2013 06:38 PM, Petr Viktorin wrote: On 01/17/2013 06:27 PM, Petr Viktorin wrote: Hello, This is the first batch of changes aimed to consolidate our LDAP code. Each should be a self-contained change that doesn't break anything. [...] Since this patchset is becoming unwieldy, I've put it in a public repo that I'll keep updated. The following command will fetch it into your pviktori-ldap-refactor branch: git fetch git://github.com/encukou/freeipa ldap-refactor:pviktori-ldap-refactor [...] I found a bug in patch 143, here is a fixed version. I would prefer if you used the semantics of .get() for .get_single() as well (i.e. when no default value is provided, None is assumed) in patch 152. Or is there a reason not to? Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 01/29/2013 04:39 PM, Petr Viktorin wrote: On 01/28/2013 04:09 PM, Petr Viktorin wrote: On 01/28/2013 09:34 AM, Jan Cholasta wrote: On 25.1.2013 14:54, Petr Viktorin wrote: On 01/24/2013 03:06 PM, Petr Viktorin wrote: On 01/24/2013 10:43 AM, Petr Viktorin wrote: On 01/22/2013 04:04 PM, Petr Viktorin wrote: On 01/21/2013 06:38 PM, Petr Viktorin wrote: On 01/17/2013 06:27 PM, Petr Viktorin wrote: Hello, This is the first batch of changes aimed to consolidate our LDAP code. Each should be a self-contained change that doesn't break anything. These patches do some general cleanup (some of the changes might seem trivial but help a lot when grepping through the code); merge the common parts LDAPEntry, Entry and Entity classes; and move stuff that depends on an installed server out of IPASimpleLDAPObject and SchemaCache. I'm posting them early so you can see where I'm going, and so you can find out if your work will conflict with mine. Here is a third set of patches. These apply on top of jcholast's patches 94-96. I found mistakes in two of the patches, attaching fixed versions. Since this patchset is becoming unwieldy, I've put it in a public repo that I'll keep updated. The following command will fetch it into your pviktori-ldap-refactor branch: git fetch git://github.com/encukou/freeipa ldap-refactor:pviktori-ldap-refactor I don't think patch 139 is necessary, I fixed this problem in patch 95 by not including 'dn' as attribute in _entry_to_entity. You're right. I'm retiring patch 139. We'll need to use entry.dn everywhere, and add an assert so that entry['dn'] is never set. Here is a fourth set of patches. Honza noticed test failures caused by patch 143. Patch 123 grew a conflict with master. Fixes attached. Today, patch 144 had a merge conflict. -- Petr³ From a12814e6b7cdd22641ef95fe8417d2bde3ae7c07 Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Mon, 21 Jan 2013 06:05:07 -0500 Subject: [PATCH] Remove unused imports from ipaserver/install Part of the work for: https://fedorahosted.org/freeipa/ticket/2660 --- ipaserver/install/adtrustinstance.py |9 - ipaserver/install/bindinstance.py | 15 ++- ipaserver/install/cainstance.py|8 +--- ipaserver/install/certs.py | 16 +++- ipaserver/install/httpinstance.py |5 ++--- ipaserver/install/installutils.py |5 - ipaserver/install/ntpinstance.py |1 - ipaserver/install/plugins/baseupdate.py|1 - ipaserver/install/plugins/dns.py |1 - .../install/plugins/fix_replica_agreements.py |2 +- ipaserver/install/plugins/rename_managed.py|5 + ipaserver/install/service.py | 14 +++--- 12 files changed, 25 insertions(+), 57 deletions(-) diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py index 425d69f08fa01067bb90f0fa30d74c139f9b8564..a2061fd74ea5a8dd055359726dd3f30f3a54098f 100644 --- a/ipaserver/install/adtrustinstance.py +++ b/ipaserver/install/adtrustinstance.py @@ -22,7 +22,10 @@ import errno import ldap import tempfile import uuid -from ipaserver.install import installutils +import string +import struct +import re + from ipaserver.install import service from ipaserver.install.dsinstance import realm_to_serverid from ipaserver.install.bindinstance import get_rr, add_rr, del_rr, \ @@ -33,13 +36,9 @@ from ipapython import sysrestore from ipapython import ipautil from ipapython.ipa_log_manager import * from ipapython import services as ipaservices -from ipapython.dn import DN import ipaclient.ipachangeconf -import string -import struct -import re ALLOWED_NETBIOS_CHARS = string.ascii_uppercase + string.digits diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py index a528320c8129f1ccfcb3837867165f4891362aa1..123f559aa0ad4b45353d3c4c98f3e384717829f1 100644 --- a/ipaserver/install/bindinstance.py +++ b/ipaserver/install/bindinstance.py @@ -23,25 +23,22 @@ import pwd import netaddr import re -import installutils import ldap + +import installutils import service from ipaserver import ipaldap from ipaserver.install.dsinstance import realm_to_serverid from ipaserver.install.cainstance import IPA_CA_CNAME -from ipaserver.install.installutils import resolve_host from ipapython import sysrestore from ipapython import ipautil -from ipalib.parameters import IA5Str +from ipapython.ipa_log_manager import * +from ipapython.dn import DN +import ipalib +from ipalib import api, errors from ipalib.util import (validate_zonemgr, normalize_zonemgr, get_dns_forward_zone_update_policy, get_dns_reverse_zone_update_policy, normalize_zone, get_reverse_zone_default, zone_is_reverse) -from ipapython.ipa_log_manager import * -from ipalib.text
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 01/28/2013 04:09 PM, Petr Viktorin wrote:
On 01/28/2013 09:34 AM, Jan Cholasta wrote:
On 25.1.2013 14:54, Petr Viktorin wrote:
On 01/24/2013 03:06 PM, Petr Viktorin wrote:
On 01/24/2013 10:43 AM, Petr Viktorin wrote:
On 01/22/2013 04:04 PM, Petr Viktorin wrote:
On 01/21/2013 06:38 PM, Petr Viktorin wrote:
On 01/17/2013 06:27 PM, Petr Viktorin wrote:
Hello,
This is the first batch of changes aimed to consolidate our LDAP
code.
Each should be a self-contained change that doesn't break anything.
These patches do some general cleanup (some of the changes might
seem
trivial but help a lot when grepping through the code); merge the
common
parts LDAPEntry, Entry and Entity classes; and move stuff that
depends
on an installed server out of IPASimpleLDAPObject and SchemaCache.
I'm posting them early so you can see where I'm going, and so you
can
find out if your work will conflict with mine.
Here is a third set of patches. These apply on top of jcholast's
patches
94-96.
I found mistakes in two of the patches, attaching fixed versions.
Since this patchset is becoming unwieldy, I've put it in a public repo
that I'll keep updated. The following command will fetch it into your
pviktori-ldap-refactor branch:
git fetch git://github.com/encukou/freeipa
ldap-refactor:pviktori-ldap-refactor
I don't think patch 139 is necessary, I fixed this problem in patch 95
by not including 'dn' as attribute in _entry_to_entity.
You're right. I'm retiring patch 139.
We'll need to use entry.dn everywhere, and add an assert so that
entry['dn'] is never set.
Here is a fourth set of patches.
Honza noticed test failures caused by patch 143. Patch 123 grew a
conflict with master. Fixes attached.
--
Petr³
From 90b4c802d3bdeeb55f173568244365b401e03610 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Wed, 23 Jan 2013 06:38:32 -0500
Subject: [PATCH] Change {add,update,delete}_entry to take LDAPEntries
These methods currently take (dn, entry_attrs, normalize=True)
(or (dn, normalize=True) for delete).
Change them to also accept just an LDAPEntry, and document the
legacy calling style as such.
Part of the work for: https://fedorahosted.org/freeipa/ticket/2660
---
ipaserver/ipaldap.py | 81 +
1 files changed, 54 insertions(+), 27 deletions(-)
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 27016e92f9435461aedee98ecb82482913d0e435..3353a5dc8e9a27368c6193ca077ec69884cdc613 100644
--- a/ipaserver/ipaldap.py
+++ b/ipaserver/ipaldap.py
@@ -1354,21 +1354,40 @@ class LDAPConnection(object):
self.log.debug(get_members: result=%s, entries)
return entries
-def add_entry(self, dn, entry_attrs, normalize=True):
-Create a new entry.
-
-assert isinstance(dn, DN)
-
-if normalize:
-dn = self.normalize_dn(dn)
-# remove all None or [] values, python-ldap hates'em
-entry_attrs = dict(
-# FIXME, shouldn't these values be an error?
-(k, v) for (k, v) in entry_attrs.iteritems()
-if v is not None and v != []
-)
+def _get_dn_and_attrs(self, entry_or_dn, entry_attrs, normalize):
+Helper for legacy calling style for {add,update}_entry
+
+if entry_attrs is None:
+assert normalize is None
+return entry_or_dn.dn, entry_or_dn
+else:
+assert isinstance(entry_or_dn, DN)
+if normalize is None or normalize:
+entry_or_dn = self.normalize_dn(entry_or_dn)
+entry_attrs = dict(entry_attrs)
+for key, value in entry_attrs.items():
+if value is None:
+entry_attrs[key] = []
+return entry_or_dn, entry_attrs
+
+def add_entry(self, entry, entry_attrs=None, normalize=None):
+Create a new entry.
+
+This should be called as add_entry(entry).
+
+The legacy two/three-argument variant is:
+add_entry(dn, entry_attrs, normalize=True)
+
+dn, attrs = self._get_dn_and_attrs(entry, entry_attrs, normalize)
+
+# remove all [] values (python-ldap hates 'em)
+attrs = dict((k, v) for k, v in attrs.iteritems()
+# FIXME: Once entry values are always lists, this condition can
+# be just if v:
+if v is not None and v != [])
+
try:
-self.conn.add_s(dn, list(entry_attrs.iteritems()))
+self.conn.add_s(dn, list(attrs.iteritems()))
except _ldap.LDAPError, e:
self.handle_errors(e)
@@ -1455,16 +1474,15 @@ class LDAPConnection(object):
return modlist
-def update_entry(self, dn, entry_attrs, normalize=True):
-
-Update entry's attributes.
+def update_entry(self, entry, entry_attrs=None, normalize=None):
+Update entry's attributes.
-An attribute value set to None deletes all current
