[Freeipa-devel] [PATCH] jderose 027 Extensible return values

[Pavel Zůna]

Rob Crittenden wrote:
> Jason Gerard DeRose wrote:
>> On Wed, 2009-11-25 at 12:05 -0500, Rob Crittenden wrote:
>>> This is purely from reading the patch, I haven't applied and tested 
>>> it yet.
>>>
>>> ipalib/output.py:
>>> +primary_key = Output('primary_key', unicode,
>>> +    'The primary key of the deleted entry'
>>> +)
>>>
>>> This isn't only for deleted entries, right?
>>
>> Ah, yeah, that should be made more generic.  This doc message is only
>> used by developers, though.
>>
>>> This import doesn't seem to be used:
>>> from inspect import getdoc
>>>
>>> What is dont_output_for_cli()? Is this an effort to make things work 
>>> while we're in transition?
>>
>> Yeah, I just renamed some methods so we can reference how they were
>> implemented.  Temporary.
>>
>>> You seem to have disabled the raw option in LDAPSearch, was that 
>>> intentional?
>>
>> Originally I got the impression we weren't going to keep both --raw and
>> --all, but this can be changed.
>>
>>> Is cli_name being dropped for label? I'm ok with that but should we 
>>> remove it from all the plugins?
>>
>> No, here is how they work:
>>
>> `cli_name` is used for the optparse names and defaults to Param.name,
>> like:
>>
>>   --first
>>
>> `label` is a human readable, translatable string.  It's used in the
>> webUI, and to prompt show entries on cli, like:
>>
>>   First name: John Doe
>>
>> `doc` is human readable help passed to optparse.make_option(help=doc).
>> It default to the value of the label.  It's used like this:
>>
>>   --uid=INT        UID (use this option to set it manually)
>>
>> In the above case the `label` is "UID" (not shown) but the `doc` is this
>> longer string.
>>
>> The user plugins provide good examples of how I think these should be
>> used.
>>
>> I'll submit a patch later documented these different string uses.
>>
>>> rob
>>
> 
> We'll also need to determine what we'll do about all the plugins. The 
> cert plugin, for example, isn't ported to this new return value system 
> and blows up in many places.
> 
> There are also some labels missing, such as for fqdn in the host plugin.
> 
> These are both quite easy to fix, I think we just need to coordinate 
> things. Perhaps if Pavel and I split up the plugins and fix anything 
> that needs fixing and commit all the patches at one time to avoid any 
> period of breakage.
> 
> rob

Just did a fast forward through the big patch. It looks mostly OK, but 
as Rob said - it breaks a few things. I don't mind fixing all the 
plugins - it shouldn't be too hard, because at this point most of them 
are just extensions of baseldap.py classes. I'm going to apply the patch 
on my tree and see what I can do in the second half of this week.

One thing I noticed:

+        return dict(
+            result=entry_attrs,
+            primary_key=keys[0],
+        )

This will work on most plugins, but you should use keys[-1], because 
keys might contain parent object keys as well. The last key is always 
the primary key of the object in question.

Pavel