[Freeipa-devel] [WIP] Thin client

[Martin Basti]

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.

Patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0485-fix-pylint-false-positive-errors-related-to-thin-cli.patch
Type: text/x-patch
Size: 1022 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160525/cdd4aa38/attachment.bin>