[Freeipa-devel] [WIP] Thin client
Pavel Vomacka
pvomacka at redhat.com
Sat May 28 16:33:02 UTC 2016
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.
>
>
>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160528/5b95337d/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: httpd.log
Type: text/x-log
Size: 5135 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160528/5b95337d/attachment.bin>
More information about the Freeipa-devel
mailing list