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

[Martin Basti]

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!

-- 
Martin Basti