[Freeipa-devel] [WIP] Thin client
Jan Cholasta
jcholast at redhat.com
Fri May 27 05:49:30 UTC 2016
On 26.5.2016 18:43, Martin Basti wrote:
>
>
> On 26.05.2016 11:21, Martin Basti wrote:
>>
>>
>> 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
>>
>>
>>
>>
>>
> Can I push that fix for pylint?
Sure, ACK.
>
> I found something suspicious in tests, did anything in IPA messages
> change? I suspect that this is related to client_patches.
Yes, 'data' is a dict which contains structured message data. I did not
see these specific failures before, though. Just add it to expected
results where missing.
>
> E AssertionError: assert_deepequal: dict keys mismatch.
> E 0026: permission_find: Search for u'testperm' with a
> limit of 1 (truncated) with members
> E missing keys = []
> E extra keys = [u'data']
> E expected = {'message': u'Search result has been
> truncated: Configured size limit exceeded', 'code': 13017, 'type':
> u'warning', 'name': u'SearchResultTruncated'}
> E got = {u'data': {u'reason': u'Configured size limit
> exceeded'}, u'message': u'Search result has been truncated: Configured
> size limit exceeded', u'code': 13017, u'type': u'warning', u'name':
> u'SearchResultTruncated'}
> E path = ('messages', 0)
>
>
>
> E AssertionError: assert_deepequal: dict keys mismatch.
> E 0024: permission_find: Search for u'testperm' with a
> limit of 1 (truncated) with members
> E missing keys = []
> E extra keys = [u'data']
> E expected = {'message': u'Search result has been
> truncated: Configured size limit exceeded', 'code': 13017, 'type':
> u'warning', 'name': u'SearchResultTruncated'}
> E got = {u'data': {u'reason': u'Configured size limit
> exceeded'}, u'message': u'Search result has been truncated: Configured
> size limit exceeded', u'code': 13017, u'type': u'warning', u'name':
> u'SearchResultTruncated'}
> E path = ('messages', 0)
--
Jan Cholasta
More information about the Freeipa-devel
mailing list