[Freeipa-devel] [PATCH] 1112 Add service constraint delegation plugin

[Jan Cholasta]

Dne 3.6.2015 v 11:34 Martin Basti napsal(a):
> 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.com at 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!
>

Pushed to master: a92328452dced34d6d6df7ad6fe585563bb909f6

-- 
Jan Cholasta