[Freeipa-devel] [WIP] Thin client
Martin Basti
mbasti at redhat.com
Fri May 27 07:26:21 UTC 2016
On 27.05.2016 07:49, Jan Cholasta wrote:
> 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.
Pushed to master: aa4123d852d81b908cd18208577bb509fff08c5b
>
>>
>> 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)
>
More information about the Freeipa-devel
mailing list