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.

-- 
Petr^2 Spacek
ipa: DEBUG: importing all plugin modules in ipalib.plugins...
ipa: DEBUG: importing plugin module ipalib.plugins.aci
ipa: DEBUG: importing plugin module ipalib.plugins.automember
ipa: DEBUG: importing plugin module ipalib.plugins.automount
ipa: DEBUG: importing plugin module ipalib.plugins.baseldap
ipa: DEBUG: ipalib.plugins.baseldap is not a valid plugin module
ipa: DEBUG: importing plugin module ipalib.plugins.baseuser
ipa: DEBUG: importing plugin module ipalib.plugins.batch
ipa: DEBUG: importing plugin module ipalib.plugins.caacl
ipa: DEBUG: importing plugin module ipalib.plugins.cert
ipa: DEBUG: importing plugin module ipalib.plugins.certprofile
ipa: DEBUG: importing plugin module ipalib.plugins.config
ipa: DEBUG: importing plugin module ipalib.plugins.delegation
ipa: DEBUG: importing plugin module ipalib.plugins.dns
ipa: DEBUG: importing plugin module ipalib.plugins.domainlevel
ipa: DEBUG: importing plugin module ipalib.plugins.group
ipa: DEBUG: importing plugin module ipalib.plugins.hbacrule
ipa: DEBUG: importing plugin module ipalib.plugins.hbacsvc
ipa: DEBUG: importing plugin module ipalib.plugins.hbacsvcgroup
ipa: DEBUG: importing plugin module ipalib.plugins.hbactest
ipa: DEBUG: importing plugin module ipalib.plugins.host
ipa: DEBUG: importing plugin module ipalib.plugins.hostgroup
ipa: DEBUG: importing plugin module ipalib.plugins.idrange
ipa: DEBUG: importing plugin module ipalib.plugins.idviews
ipa: DEBUG: importing plugin module ipalib.plugins.internal
ipa: DEBUG: importing plugin module ipalib.plugins.krbtpolicy
ipa: DEBUG: importing plugin module ipalib.plugins.migration
ipa: DEBUG: importing plugin module ipalib.plugins.misc
ipa: DEBUG: importing plugin module ipalib.plugins.netgroup
ipa: DEBUG: importing plugin module ipalib.plugins.otpconfig
ipa: DEBUG: importing plugin module ipalib.plugins.otptoken
ipa: DEBUG: importing plugin module ipalib.plugins.otptoken_yubikey
ipa: DEBUG: importing plugin module ipalib.plugins.passwd
ipa: DEBUG: importing plugin module ipalib.plugins.permission
ipa: DEBUG: importing plugin module ipalib.plugins.ping
ipa: DEBUG: importing plugin module ipalib.plugins.pkinit
ipa: DEBUG: importing plugin module ipalib.plugins.privilege
ipa: DEBUG: importing plugin module ipalib.plugins.pwpolicy
ipa: DEBUG: Starting external process
ipa: DEBUG: args=klist -V
ipa: DEBUG: Process finished, return code=0
ipa: DEBUG: stdout=Kerberos 5 version 1.14.1

ipa: DEBUG: stderr=
ipa: DEBUG: importing plugin module ipalib.plugins.radiusproxy
ipa: DEBUG: importing plugin module ipalib.plugins.realmdomains
ipa: DEBUG: importing plugin module ipalib.plugins.role
ipa: DEBUG: importing plugin module ipalib.plugins.rpcclient
ipa: DEBUG: importing plugin module ipalib.plugins.selfservice
ipa: DEBUG: importing plugin module ipalib.plugins.selinuxusermap
ipa: DEBUG: importing plugin module ipalib.plugins.server
ipa: DEBUG: importing plugin module ipalib.plugins.service
ipa: DEBUG: importing plugin module ipalib.plugins.servicedelegation
ipa: DEBUG: importing plugin module ipalib.plugins.session
ipa: DEBUG: importing plugin module ipalib.plugins.stageuser
ipa: DEBUG: importing plugin module ipalib.plugins.sudocmd
ipa: DEBUG: importing plugin module ipalib.plugins.sudocmdgroup
ipa: DEBUG: importing plugin module ipalib.plugins.sudorule
ipa: DEBUG: importing plugin module ipalib.plugins.topology
ipa: DEBUG: importing plugin module ipalib.plugins.trust
ipa: DEBUG: importing plugin module ipalib.plugins.user
ipa: DEBUG: importing plugin module ipalib.plugins.vault
ipa: DEBUG: importing plugin module ipalib.plugins.virtual
ipa: DEBUG: ipalib.plugins.virtual is not a valid plugin module
ipa: DEBUG: raw: help(None, outfile=<_io.TextIOWrapper name='<stderr>' mode='w' encoding='UTF-8'>, version='2.169')
ipa: DEBUG: help(None, outfile=<_io.TextIOWrapper name='<stderr>' mode='w' encoding='UTF-8'>, version='2.169')
ipa: ERROR: TypeError: run() missing 1 required positional argument: 'key'
Traceback (most recent call last):
  File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1348, in run
    sys.exit(api.Backend.cli.run(argv))
  File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1096, in run
    cmd = self.get_command(argv)
  File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1065, in get_command
    self.Command.help(outfile=sys.stderr)
  File "/usr/lib/python3.5/site-packages/ipalib/frontend.py", line 440, in __call__
    return self.__do_call(*args, **options)
  File "/usr/lib/python3.5/site-packages/ipalib/frontend.py", line 468, in __do_call
    ret = self.run(*args, **options)
TypeError: run() missing 1 required positional argument: 'key'
ipa: ERROR: an internal error has occurred
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to