[Freeipa-devel] [PATCH] 1112 Add service constraint delegation plugin
Martin Basti
mbasti at redhat.com
Tue Jun 2 14:03:41 UTC 2015
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.com at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0000-review-service-delegation.patch
Type: text/x-patch
Size: 6160 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150602/fc80a207/attachment.bin>
More information about the Freeipa-devel
mailing list