On 02/06/15 22:51, Rob Crittenden wrote:
Martin Basti wrote:
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

Fixed.



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

Fixed. I'm tempted to just drop this regex entirely. Other plugins have no such restrictions, but this should work better now.


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.

Yeah, a copy and paste issue.

Patch with minor fixes attached.

I removed unused code and PEP8 complains

Incorporated and fixed a number of other things, including some typos in the doc examples.

rob



Thank you, ACK!

--
Martin Basti

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