[Freeipa-devel] [WIP] Thin client

[David Kupka]

On 18/05/16 08:01, Jan Cholasta wrote:
> On 16.5.2016 16:35, Martin Basti wrote:
>>
>>
>> On 09.05.2016 14:07, Jan Cholasta wrote:
>>> On 6.5.2016 14:32, Martin Basti wrote:
>>>>
>>>>
>>>> On 28.04.2016 14:45, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> I have pushed my thin client WIP branch to GitHub:
>>>>> <https://github.com/jcholast/freeipa/tree/trac-4739>.
>>>>>
>>>>> All commits up to "ipalib: use relative imports for cross-plugin
>>>>> imports" should be good for review. The rest is subject to change
>>>>> (WARNING: I will force push into this branch).
>>>>>
>>>>> Honza
>>>>>
>>>> I did not test it yet, I just checked the code
>>>>
>>>> * automount: do not inherit automountlocation_tofiles from LDAPQuery *
>>>> LGTM
>>>>
>>>> * dns: move all dnsrecord code called on client to a single class *
>>>> LGTM
>>>>
>>>> * dns: do not rely on server data structures in code called on client *
>>>> 1)
>>>> you forgot to increment VERSION
>>>
>>> This was deliberate, as it will no longer be necessary to bump VERSION
>>> for backward compatible changes after this whole patchset is merged.
>>> But we're not there yet, so fixed.
>>>
>> How we should handle VERSION after your patches?
>>>>
>>>> Otherwise LGTM
>>>>
>>>> * otptoken: fix import of DN *
>>>> ACK
>>>>
>>>> * otptoken_yubikey: fix otptoken_add_yubikey arguments *
>>>> 1)
>>>> you forgot to increment VERSION
>>>
>>> Fixed.
>>>
>>>>
>>>> 2)
>>>> Did you find out why this was issue?
>>>> -        del answer['value']    # Why does this cause an error if
>>>> omitted?
>>>>  -        del answer['summary']  # Why does this cause an error if
>>>> omitted?
>>>
>>> The command definition was not complete, it was missing has_output.
>>>
>>>>
>>>> Otherwise LGTM
>>>>
>>>> * vault: move client-side code to the module level *
>>>> LGTM
>>>>
>>>> * vault: copy arguments of client commands from server counterparts *
>>>> 1)
>>>> you forgot to increment Version
>>>
>>> Fixed.
>>>
>>>>
>>>> * ipalib: use relative imports for cross-plugin imports *
>>>> 1) Missing explanation for future generations why this change is needed
>>>> in commit message
>>>
>>> Fixed.
>>>
>>>>
>>>> The other commits I will check later.
>>>> Martin^2
>>>>
>>>
>>> Okay. Thanks.
>>>
>>
>> * frontend: remove the unused Command.soft_validate method
>> LGTM
>>
>> * frontend: perform argument value validation only on server
>> LGTM
>>
>> * frontend: do not forward argument defaults to server
>> I'm not a fan of returning  None in promt_param function, but I havent
>> found anything better to use.
>
> It always returned None for unset params.
>
>> LGTM
>>
>> * ipalib: optimize API.txt
>> this contains a lot of black magic, but because this is mainly copy of
>> current to proper places, LGTM
>
> It's actually mostly cut-n-paste.
>
>>
>> * makeaci: load additional plugins using API.add_module
>> Looks good, but I miss explanation why is this change needed
>
> The next change would be impossible without this.
>
>>
>> * plugable: replace API.import_plugins with new API.add_package
>> LGTM
>>
>>
>> * ipalib, ipaserver: migrate all plugins to Registry-based registration
>> ACK
>>
>> * ipalib, ipaserver: fix incorrect API.register calls in docstrings
>> LGTM
>>
>>
>> * plugable: remove the unused deprecated API.register method
>> LGTM
>>
>>
>> * plugable: switch API to Registry-based plugin discovery
>> LGTM
>>
>> * frontend: merge baseldap.CallbackRegistry into Command
>> LGTM
>>
>> *frontend: move the interactive_prompt callback type to Command
>>  LGTM
>>
>> Martin^2
>>
>>
>
>

Hello,
first batch of 30 patches from Honza's trac-4739 branch 
(https://github.com/jcholast/freeipa/commits/trac-4739) is ready to be 
pushed into master.
All upto "frontend: allow commands to have an argument named `name`" got 
over numerous test&fix cycles in last week+ and is working as expected 
now, ACK.

Honzo, please rebase and push them, thanks!

-- 
David Kupka