On 31/05/15 04:07, Rob Crittenden wrote:
Petr Vobornik wrote:
On 05/27/2015 08:17 PM, Martin Basti wrote:
On 27/05/15 19:27, Rob Crittenden wrote:
Martin Basti wrote:


Thank you.

I haven't finished review yet, but I have few notes in case you will
modify the patch.

Please fix following issues:


3)
There are many PEP8 errors, can you fix some of them,?

Is PEP8 a concern? What kinds of errors do we fix? For example, the
current model for defining options generates a slew of indention errors.

In old modules it's preferred to keep the old indentation style for
options(not to mix 2 styles). New modules should use following pep8
compliant style:
         Str(
             'cn',
             cli_name='name',
             primary_key=True,
             label=_('Server name'),
             doc=_('IPA server hostname'),
         ),

We try to keep PEP8 in new code, mainly indentation, blank lines, too
long lines.
Yes in test definitions and option definitions, is better to keep the
same style, but other parts of code should be PEP8.

For example these should be fixed
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:37:13: E225
missing whitespace around operator
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:39:1: E302
expected 2 blank lines, found 1
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:42:1: E302
expected 2 blank lines, found 1



I'll wait and see what falls out of the API review before making any
real changes.

rob

Updated API and addressed Martin's concerns. The regex must have been a bad copy/paste, it is fixed now.

The design page has been updated as well.

rob

Hello,

comments below, in the right thread:

1)
+    Str(
+        'memberprincipal',
+        label=_('Failed principals'),
+    ),
+    Str(
+        'ipaallowedtarget',
+        label=_('Failed targets'),
+    ),
+    Str(
+        'servicedelegationrule',
+        label=_('principal member'),
+    ),
Are these names correct?
# ipa servicedelegationrule-find
----------------------------------
1 service delegation rule matched
----------------------------------
   Delegation name: ipa-http-delegation
Allowed Target: ipa-ldap-delegation-targets, ipa-cifs-delegation-targets
   Failed principals: HTTP/vm-093.example....@example.com


2)
+ pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$',
+            pattern_errmsg='may only include letters, numbers, _, -, ., '
+                           'and a space inside',

This regex does not allow space inside
In [6]: print re.match(pattern, 'lalalala lalala')
None


3)
+            yield Str('%s*' % name, cli_name='%ss' % name, doc=doc,
+                      label=_('member %s') % name,
+                      csv=True, alwaysask=True)

IMHO CSV values should not be supported.
Honza told me, the option doesn't work anyway.

Patch with minor fixes attached.

I removed unused code and PEP8 complains

--
Martin Basti

From 3d051d3ec13fe19e2e5246b28e848460538d81bf Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Tue, 2 Jun 2015 15:49:21 +0200
Subject: [PATCH] review service delegation

---
 ipalib/plugins/servicedelegation.py | 40 ++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/ipalib/plugins/servicedelegation.py b/ipalib/plugins/servicedelegation.py
index 434c446de590299b8a1c26344cbecd17f6a8ef3c..50b07dc8ad3112c0ee636b0d0ea7e0cd59365d9c 100644
--- a/ipalib/plugins/servicedelegation.py
+++ b/ipalib/plugins/servicedelegation.py
@@ -178,8 +178,7 @@ class servicedelegation_add_member(LDAPAddMember):
             name = self.member_names[attr]
             doc = self.member_param_doc % name
             yield Str('%s*' % name, cli_name='%ss' % name, doc=doc,
-                      label=_('member %s') % name,
-                      csv=True, alwaysask=True)
+                      label=_('member %s') % name, alwaysask=True)
 
     def get_member_dns(self, **options):
         """
@@ -187,7 +186,7 @@ class servicedelegation_add_member(LDAPAddMember):
         special handling since it is just a principal, not a
         full dn.
         """
-        return (dict(), dict())
+        return dict(), dict()
 
     def post_callback(self, ldap, completed, failed, dn, entry_attrs,
                       *keys, **options):
@@ -210,8 +209,8 @@ class servicedelegation_add_member(LDAPAddMember):
                 name = normalize_principal(name)
                 obj_dn = ldap_obj.get_dn(name)
                 try:
-                    s_attrs = ldap.get_entry(obj_dn, ['krbprincipalname'])
-                except errors.NotFound, e:
+                    ldap.get_entry(obj_dn, ['krbprincipalname'])
+                except errors.NotFound as e:
                     failed[attr][ldap_obj_name].append((name, unicode(e)))
                     continue
                 try:
@@ -219,22 +218,21 @@ class servicedelegation_add_member(LDAPAddMember):
                         members.append(name)
                     else:
                         raise errors.AlreadyGroupMember()
-                except errors.PublicError, e:
+                except errors.PublicError as e:
                     failed[attr][ldap_obj_name].append((name, unicode(e)))
                 else:
                     completed += 1
 
         if members:
-            if attr in entry_attrs:
-                entry_attrs[attr] += members
-            else:
-                entry_attrs[attr] = members
+            value = entry_attrs.setdefault(attr, [])
+            value.extend(members)
+
             try:
                 ldap.update_entry(entry_attrs)
             except errors.EmptyModlist:
                 pass
 
-        return (completed, dn)
+        return completed, dn
 
 
 class servicedelegation_remove_member(LDAPRemoveMember):
@@ -253,8 +251,7 @@ class servicedelegation_remove_member(LDAPRemoveMember):
             name = self.member_names[attr]
             doc = self.member_param_doc % name
             yield Str('%s*' % name, cli_name='%ss' % name, doc=doc,
-                      label=_('member %s') % name,
-                      csv=True, alwaysask=True)
+                      label=_('member %s') % name, alwaysask=True)
 
     def get_member_dns(self, **options):
         """
@@ -282,9 +279,9 @@ class servicedelegation_remove_member(LDAPRemoveMember):
                     ldap_obj = self.api.Object[ldap_obj_name]
                     try:
                         dns[attr][ldap_obj_name].append(ldap_obj.get_dn(name))
-                    except errors.PublicError, e:
+                    except errors.PublicError as e:
                         failed[attr][ldap_obj_name].append((name, unicode(e)))
-        return (dns, failed)
+        return dns, failed
 
     def post_callback(self, ldap, completed, failed, dn, entry_attrs,
                       *keys, **options):
@@ -295,7 +292,6 @@ class servicedelegation_remove_member(LDAPRemoveMember):
         ldap = self.obj.backend
         ldap_obj_name = self.obj.name
         attr = 'memberprincipal'
-        members = []
         failed[attr][ldap_obj_name] = []
         names = options.get(self.member_names[attr], [])
         if names:
@@ -308,7 +304,7 @@ class servicedelegation_remove_member(LDAPRemoveMember):
                         entry_attrs[attr].remove(name)
                     else:
                         raise errors.NotGroupMember()
-                except errors.PublicError, e:
+                except errors.PublicError as e:
                     failed[attr][ldap_obj_name].append((name, unicode(e)))
                 else:
                     completed += 1
@@ -318,7 +314,7 @@ class servicedelegation_remove_member(LDAPRemoveMember):
         except errors.EmptyModlist:
             pass
 
-        return (completed, dn)
+        return completed, dn
 
 
 @register()
@@ -358,9 +354,6 @@ class servicedelegationrule_del(LDAPDelete):
 
     def pre_callback(self, ldap, dn, *keys, **options):
         assert isinstance(dn, DN)
-        delegation_attrs = self.obj.methods.show(
-            self.obj.get_primary_key_from_dn(dn), all=True
-        )['result']
         if keys[0] in PROTECTED_CONSTRAINT_RULES:
             raise errors.ProtectedEntryError(
                 label=_(u'service delegation rule'),
@@ -456,9 +449,6 @@ class servicedelegationtarget_del(LDAPDelete):
 
     def pre_callback(self, ldap, dn, *keys, **options):
         assert isinstance(dn, DN)
-        delegation_attrs = self.obj.methods.show(
-            self.obj.get_primary_key_from_dn(dn), all=True
-        )['result']
         if keys[0] in PROTECTED_CONSTRAINT_TARGETS:
             raise errors.ProtectedEntryError(
                 label=_(u'service delegation target'),
@@ -504,7 +494,7 @@ class servicedelegationtarget_find(LDAPSearch):
         sfilter = ldap.combine_filters(
             (term_filter, attr_filter), rules=ldap.MATCH_ALL
         )
-        return (sfilter, base_dn, ldap.SCOPE_ONELEVEL)
+        return sfilter, base_dn, ldap.SCOPE_ONELEVEL
 
 
 @register()
-- 
2.1.0

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