[Freeipa-devel] [WIP] Thin client
Pavel Vomacka
pvomacka at redhat.com
Mon May 30 07:48:14 UTC 2016
On 05/30/2016 08:28 AM, Jan Cholasta wrote:
> On 28.5.2016 18:33, Pavel Vomacka wrote:
>>
>>
>> On 05/27/2016 10:33 AM, Petr Spacek wrote:
>>> On 27.5.2016 09:26, Martin Basti wrote:
>>>>
>>>> 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)
>>>
>>> It is even worse that tests.
>>>
>>> If I call command 'ipa' or 'ipa help' it blows up. I'm attaching
>>> debug log
>>> from 'ipa -d' invocation.
>
> Oops, I forgot about commands defined in ipalib.cli in commit 71f9604.
>
> Will fix.
>
>>>
>>>
>>>
>> Hi Honza,
>>
>> I installed FreeIPA from rpms built from the current master branch and
>> after restarting httpd service I got Internal Server Error in WebUI. I
>> guess that it has something in common with Thin Client, so the stack
>> trace from /var/log/httpd/error_log is in attached file.
>
> This is weird, json_metadata.execute does *not* take exactly 3 arguments:
>
> def execute(self, objname=None, methodname=None, **options):
>
> Have you upgraded your server properly?
>
You are right I didn't have correct internal.py file on the server.
Thank you for your help.
More information about the Freeipa-devel
mailing list