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

[Rob Crittenden]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1112-5-Add-plugin-to-manage-service-constraint-delegations.patch
Type: text/x-diff
Size: 57147 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150602/9bd7173c/attachment.bin>