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
