[Freeipa-devel] [WIP] Thin client

[Jan Cholasta]

On 25.5.2016 18:17, Martin Basti wrote:
>
>
> On 25.05.2016 16:07, Jan Cholasta wrote:
>> On 25.5.2016 15:03, David Kupka wrote:
>>> 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.
>>
>> Thanks for the review.
>>
>>>
>>> Honzo, please rebase and push them, thanks!
>>>
>>
>> Attaching the patches for reference.
>>
>> Pushed to master: 2b50fc617024f18a81e97f30f75ed1b9221c4055
>>
>
> Guys, you forgot to test it with newer pylint.

I don't see any "BuildRequires: newer pylint" in the spec file.

>
> Patch attached.

LGTM, although the commit message is wrong - this is not related to thin 
client at all, PublicError and PublicMessage had .kw since forever.

-- 
Jan Cholasta