[Freeipa-devel] [WIP] Thin client
Martin Basti
mbasti at redhat.com
Thu May 26 16:43:23 UTC 2016
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?
I found something suspicious in tests, did anything in IPA messages
change? I suspect that this is related to client_patches.
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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160526/c597a562/attachment.htm>
More information about the Freeipa-devel
mailing list