[Freeipa-devel] [WIP] Thin client

[Jan Cholasta]

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
>
>


-- 
Jan Cholasta