[Freeipa-devel] [WIP] Thin client

Fri Jun 3 06:08:55 UTC 2016

On 25/05/16 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
>

Hello,
another patchset is ready. There are still some minor issues with 
interactive prompt in dns* commands but these can be fixed later. 
Otherwise all work as expected, ACK.

Also it would be good to have tests for schema plugin, I have filled a 
ticket [1].

List of commits in Honza's trac-4739 branch:
frontend: do not check API minor version of the client
ipalib: move server-side plugins to ipaserver
ipaclient: implement thin client
misc: hide the unused --all option of `env` and `plugins` in CLI
ipalib: move File command arguments to ipaclient
ipactl: use server API
client install: finalize API after CA certs are available
rpc: do not validate command name in RPCClient.forward
rpc: optimize JSON-RPC response handling
rpc: specify connection options in API config
rpc: allow overriding NSS DB directory in API config
rpc: respect API config in RPCClient.create_connection
ipalib: introduce API schema plugins
ipalib: replace DeprecatedParam with `deprecated` Param argument
parameters: introduce no_convert keyword argument
parameters: introduce cli_metavar keyword argument
ipalib: split off client-side plugin code into ipaclient
dns: move code shared by client and server to separate module
ipaclient: add client-side command override class
frontend: turn Method attributes into properties
plugable: remember overriden plugins in API
plugable: simplify API plugin initialization code
plugable: turn Plugin attributes into properties
help, makeapi: do not use hardcoded plugin package name
help, makeapi: specify module topic by name
help, makeapi: allow setting command topic explicitly
ipalib: move client-side plugins to ipaclient
ipaclient: introduce ipaclient.plugins
dns: fix dnsrecord interactive mode
cli: make optional positional command arguments actually optional

[1] https://fedorahosted.org/freeipa/ticket/5929
-- 
David Kupka