[Freeipa-devel] [PATCHES] Bring back old outputting functionality

[Rob Crittenden]

Pavel Zuna wrote:
> Rob Crittenden wrote:
>> Pavel Zuna wrote:
>>> I compiled 3 patches, that effectively bring back all the 
>>> functionality we had before Jasons big patch (i.e. before introducing 
>>> output validation and the common output interface).
>>>
>>> --all and --raw are back, but this time as global options
>>> replacing DNs with primary keys is back
>>> clever attribute printing (word-wrapping etc.) is back too
>>>
>>> To implement --all and --raw as global options, we had to find a way 
>>> to propagate additional information (apart from command name and 
>>> parameters) from client to server. We extended the XML-RPC signature 
>>> from:
>>>
>>> (arg0, arg1, ..., options)
>>>
>>> to:
>>>
>>> (args, options, extras)
>>>
>>> The extras dict is currently only filled with the 'print_all_attrs' 
>>> and 'print_raw_attrs' settings when forwarding a call. The server 
>>> saves the extras dict into the thread specific context variable.
>>>
>>> I also replaced the decoding table in Encoder, because it didn't 
>>> really work as expected in special cases. It now uses a dont-decode 
>>> function. In the case of ldap2, this function checks attribute type 
>>> OIDs and returns False for binary types.
>>>
>>> This patch introduces a little problem with the env command, because 
>>> it fixes a bug/feature, that made it work before. Before outputting 
>>> an attribute, we check if it isn't of type str. If it is, we assume 
>>> it is binary and decode it. All values in Env are str. I propose we 
>>> either write a specific output_for_cli for the env command or think 
>>> about switching from str to unicode. I tried the later and it didn't 
>>> cause any problems so far.
>>>
>>> How it's supposed to work:
>>>
>>> # ./ipa user-show admin
>>>   User login: admin
>>>   Last name: Administrator
>>>   Home directory: /home/admin
>>>   Login shell: /bin/bash
>>>
>>> # ./ipa --all user-show admin
>>>   dn: uid=admin,cn=users,cn=accounts,dc=pzuna
>>>   User login: admin
>>>   Last name: Administrator
>>>   Full name: Administrator
>>>   Home directory: /home/admin
>>>   GECOS field: Administrator
>>>   Login shell: /bin/bash
>>>   Kerberos principal: admin at PZUNA
>>>   UID: 1083719807
>>>   GID: 1083719807
>>>   Last password change date: 20100208132706Z
>>>   Password expiration date: 20100509132706Z
>>>   Member of groups: admins
>>>   objectclass: top, person, posixaccount, krbprincipalaux, 
>>> krbticketpolicyaux, inetuser
>>>
>>> # ./ipa --raw user-show admin
>>>   uid: admin
>>>   sn: Administrator
>>>   homedirectory: /home/admin
>>>   loginshell: /bin/bash
>>>
>>> # ./ipa --all --raw user-show admin
>>>   dn: uid=admin,cn=users,cn=accounts,dc=pzuna
>>>   uid: admin
>>>   sn: Administrator
>>>   cn: Administrator
>>>   homedirectory: /home/admin
>>>   gecos: Administrator
>>>   loginshell: /bin/bash
>>>   krbprincipalname: admin at PZUNA
>>>   uidnumber: 1083719807
>>>   gidnumber: 1083719807
>>>   krblastpwdchange: 20100208132706Z
>>>   krbpasswordexpiration: 20100509132706Z
>>>   memberof: cn=admins,cn=groups,cn=accounts,dc=pzuna
>>>   objectclass: top
>>>   objectclass: person
>>>   objectclass: posixaccount
>>>   objectclass: krbprincipalaux
>>>   objectclass: krbticketpolicyaux
>>>   objectclass: inetuser
>>>
>>> Pavel
>>
>> Generally looks ok, have some questions though:
>>
>> - We currently rely on the fact that binary objects are encoded as 
>> python str, it's how we determine what to base64-encode. What 
>> mechanism will we have to do that now?
> I didn't (and I'm not planning to) make any changes in this matter.

My point is that for binary objects we were explicitly setting their 
type to str. We don't seem to be doing that any more, so are we relying 
on python-ldap to default to the str type? It's ok if we do I'd just 
like to see a comment to that effect in case something changes in the 
future.

> What I'm saying is that the Env object stores all strings as str and the 
> env command uses the same output_for_cli as LDAP commands, that only use 
> str for binary. So, we either need to override output_for_cli or switch 
> to unicode in Env.

Not exactly sure what to do here though using unicode seems like the 
best route.

> 
>> - Is print_* the right prefix for these new global variables? It 
>> affects more than just printing in the case of all because it returns 
>> everything over XML-RPC as well.
> You're right, maybe get_* or something similar would be better. I'm open 
> to suggestions.

I'm ok with print_raw because that is what it does. maybe print_all -> 
retrieve_all?

>> - Is there/should there be a way for a plugin to define its own 
>> extras? And not to be too pedantic but is extras the best description 
>> for these values? Not that I have any suggestions for an improvement 
>> :-( Perhaps global_options?
> The extras dict is there to pass additional information, that is command 
> independent. Commands probably shouldn't define their own. I say 
> probably, because it is possible, that we're going to find out this is 
> actually the best way to accomplish something.
> 
> Extras might not be the best description, but we need something general, 
> because it can contain pretty much anything and not just global options.

Ok, I don't want to agonize too much over a variable name.

>> - Why are you removing get_options() from LDAPSearch()?
> Because it was only used to generate an option for the UUID attribute. 
> Since Jason's no_create,no_update patch it isn't needed anymore, because 
> we can just define an UUID param with these flags set.

Ok.

>> It doesn't look like this is going to conflict too much with the 
>> parallel work I've done in regard to including member/memberof in 
>> return values, nor in the output work I've done. So you don't need to 
>> work on the individual plugins at all, I've got that ready in my tree 
>> though I'm going to hold onto it until we can get these patches 
>> committed.
> Cool, that's good to hear... er, I mean read. :) I was a bit worried, 
> because on the meeting you sounded like you spent quite a bit of time 
> working on it and having to deal with conflicts/merges/andwhatnot 
> because of my work wouldn't be the most appropriate reward. :)

Yeah, minus your 4th patch the merge went incredibly smoothly. My patch 
covered most of what it did with a few exceptions on some default 
parameters.

rob