[Freeipa-devel] [WIP] Thin client
Jan Cholasta
jcholast at redhat.com
Mon May 30 06:28:40 UTC 2016
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?
--
Jan Cholasta
More information about the Freeipa-devel
mailing list