[Freeipa-devel] [WIP] Thin client

[Martin Basti]

On 26.05.2016 07:15, Jan Cholasta wrote:
> 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.
>
updated commit message



-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0485.2-fix-pylint-false-positive-errors.patch
Type: text/x-patch
Size: 999 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160526/ac895491/attachment.bin>