Hello.
AFAIK this library needs some effort to be Python 3 compatible and I just want to ask if there is some plan to do it. As I can see the biggest issues will be the move from python-ldap to py.ldap and port the whole codebase.
Let me know, if I can help you with anything. Have a nice day. Lumír
Hi there
We already have an issue open for this (sadly, it's on the 389 ds base tracker, as we are in the middle of a migration). https://pagure.io/389-ds-base/issue/48355
I have in fact already done most of the python 3 porting work. There have been some issues with pyldap on python3, but we have mostly developed work arounds for these. I mostly run with python 3 builds internally to help facilitate this, but recently an unrelated fedora GCC6 issue has forced me back to rhel 7 until I can resolve it.
At this stage, it would not be hard to do the "final push" to finish this, and then convert our tests to be python 3 compatible too.
Metadata Update from @firstyear: - Custom field Origin adjusted to Community - Custom field Review Status adjusted to review
<img alt="0001-Ticket-lib389-3-python-3-support.patch" src="/lib389/issue/raw/files/351a677d39ec53c54cdfccee730f8f40be0500e09d585dd11ca8da73235215d2-0001-Ticket-lib389-3-python-3-support.patch" />
<img alt="0001-Ticket-lib389-3-python-3-support.patch" src="/lib389/issue/raw/files/7e7070cb03f6b15a3f4ced17b707718113e518c7aed3d9c238548a598a531335-0001-Ticket-lib389-3-python-3-support.patch" />
LGTM, thought one small issue.
As long as you use 'test_user = UserAccount(topo_m4.ms["master1"], TEST_ENTRY_DN)' in test_entry fixture, you can 'return test_user' in the end of it. And then in the test cases, you can use 'test_entry.remove()', 'test_entry.add()', etc.
Probably, you can use it like this in two functions where you need it from master1. In other places, you are right, it isn't comfortable.
You have my ack anyway.
Metadata Update from @spichugi: - Custom field Review Status adjusted to ack (was: review)
Working on ACL suite now.
commit e8c56aaf2eadef1cc9aa6e8e39231ac9e31776c6 To ssh://git@pagure.io/389-ds-base.git bc6dbf1..e8c56aa master -> master
<img alt="0001-Issue-lib389-3-Python-3-support-for-ACL-test-suite.patch" src="/lib389/issue/raw/files/f5707f366a5769c0232aaa0a0270db72372740c449b681c48a567c3e7523d37d-0001-Issue-lib389-3-Python-3-support-for-ACL-test-suite.patch" />
Ack from me,
@spichugi , I have some failures with your patch:
=========================================================================== ERRORS ============================================================================ __________________________________________________________ ERROR at setup of test_rdn_write_get_ger ___________________________________________________________ topology_m2 = <lib389.topologies.TopologyMain object at 0x7fc9115f3eb8> @pytest.fixture(scope="module") def rdn_write_setup(topology_m2): topology_m2.ms["master1"].log.info("\n\n######## Add entry tuser ########\n") topology_m2.ms["master1"].add_s(Entry((SRC_ENTRY_DN, { 'objectclass': "top person".split(), 'sn': SRC_ENTRY_CN, > 'cn': SRC_ENTRY_CN}))) suites/acl/acl_test.py:1063: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:158: in inner return f(ent.dn, ent.toTupleList(), *args[2:]) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:404: in add_s return self.add_ext_s(dn,modlist,None,None) /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:160: in inner return f(*args, **kwargs) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:390: in add_ext_s resp_type, resp_data, resp_msgid, resp_ctrls = self.result3(msgid,all=1,timeout=self.timeout) /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:162: in inner return f(*args, **kwargs) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:714: in result3 resp_ctrl_classes=resp_ctrl_classes /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:162: in inner return f(*args, **kwargs) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:721: in result4 ldap_result = self._ldap_call(self._l.result4,msgid,all,timeout,add_ctrls,add_intermediates,add_extop) /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:162: in inner return f(*args, **kwargs) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <lib389.DirSrv object at 0x7fc90eff10f0>, func = <built-in method result4 of LDAP object at 0x7fc90ed95058>, args = (275, 1, -1, 0, 0, 0), kwargs = {} diagnostic_message_success = None def _ldap_call(self,func,*args,**kwargs): """ Wrapper method mainly for serializing calls into OpenLDAP libs and trace logs """ self._ldap_object_lock.acquire() if __debug__: if self._trace_level>=1: self._trace_file.write('*** %s %s - %s\n%s\n' % ( repr(self), self._uri, '.'.join((self.__class__.__name__,func.__name__)), pprint.pformat((args,kwargs)) )) if self._trace_level>=9: traceback.print_stack(limit=self._trace_stack_limit,file=self._trace_file) diagnostic_message_success = None try: try: > result = func(*args,**kwargs) E ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'add' privilege to add the entry 'cn=tuser,dc=example,dc=com'.\n"} /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:294: INSUFFICIENT_ACCESS -------------------------------------------------------------------- Captured stderr setup -------------------------------------------------------------------- INFO:lib389: ######## Add entry tuser ######## ______________________________________________________ ERROR at setup of test_rdn_write_modrdn_anonymous ______________________________________________________ topology_m2 = <lib389.topologies.TopologyMain object at 0x7fc9115f3eb8> @pytest.fixture(scope="module") def rdn_write_setup(topology_m2): topology_m2.ms["master1"].log.info("\n\n######## Add entry tuser ########\n") topology_m2.ms["master1"].add_s(Entry((SRC_ENTRY_DN, { 'objectclass': "top person".split(), 'sn': SRC_ENTRY_CN, > 'cn': SRC_ENTRY_CN}))) suites/acl/acl_test.py:1063: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:158: in inner return f(ent.dn, ent.toTupleList(), *args[2:]) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:404: in add_s return self.add_ext_s(dn,modlist,None,None) /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:160: in inner return f(*args, **kwargs) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:390: in add_ext_s resp_type, resp_data, resp_msgid, resp_ctrls = self.result3(msgid,all=1,timeout=self.timeout) /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:162: in inner return f(*args, **kwargs) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:714: in result3 resp_ctrl_classes=resp_ctrl_classes /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:162: in inner return f(*args, **kwargs) /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:721: in result4 ldap_result = self._ldap_call(self._l.result4,msgid,all,timeout,add_ctrls,add_intermediates,add_extop) /opt/rh/rh-python35/root/usr/lib/python3.5/site-packages/lib389/__init__.py:162: in inner return f(*args, **kwargs) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <lib389.DirSrv object at 0x7fc90eff10f0>, func = <built-in method result4 of LDAP object at 0x7fc90ed95058>, args = (275, 1, -1, 0, 0, 0), kwargs = {} diagnostic_message_success = None def _ldap_call(self,func,*args,**kwargs): """ Wrapper method mainly for serializing calls into OpenLDAP libs and trace logs """ self._ldap_object_lock.acquire() if __debug__: if self._trace_level>=1: self._trace_file.write('*** %s %s - %s\n%s\n' % ( repr(self), self._uri, '.'.join((self.__class__.__name__,func.__name__)), pprint.pformat((args,kwargs)) )) if self._trace_level>=9: traceback.print_stack(limit=self._trace_stack_limit,file=self._trace_file) diagnostic_message_success = None try: try: > result = func(*args,**kwargs) E ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'add' privilege to add the entry 'cn=tuser,dc=example,dc=com'.\n"} /opt/rh/rh-python35/root/usr/lib64/python3.5/site-packages/ldap/ldapobject.py:294: INSUFFICIENT_ACCESS ========================================================================== FAILURES =========================================================================== _______________________________________________________________ test_mode_legacy_ger_with_moddn _______________________________________________________________ topology_m2 = <lib389.topologies.TopologyMain object at 0x7fc9115f3eb8>, moddn_setup = None def test_mode_legacy_ger_with_moddn(topology_m2, moddn_setup): """This test checks mode legacy : GER with moddn :id: 37c1e537-1b5d-4fab-b62a-50cd8c5b3493 :setup: MMR with two masters, M1 - staging DIT M2 - production DIT add test accounts in staging DIT :steps: 1. Disable ACI checks - set nsslapd-moddn-aci: off 2. Add moddn ACI on M1 3. Search for GER controls on M1 4. Check entryLevelRights value for entries 5. Check 'n' is in the entryLevelRights 6. Try MOD with the both ACI :expectedresults: 1. It should pass 2. It should pass 3. It should pass 4. It should pass 5. It should pass 6. It should pass """ suffix = Domain(topology_m2.ms["master1"], SUFFIX) topology_m2.ms["master1"].log.info("\n\n######## Disable the moddn aci mod ########\n") _bind_manager(topology_m2) topology_m2.ms["master1"].config.set(CONFIG_MODDN_ACI_ATTR, 'off') topology_m2.ms["master1"].log.info("\n\n######## mode legacy : GER with moddn ########\n") # being allowed to read/write the RDN attribute use to allow the RDN ACI_TARGET = "(target = \"ldap:///%s\")(targetattr=\"cn\")" % (PRODUCTION_DN) ACI_ALLOW = "(version 3.0; acl \"MODDN production changing the RDN attribute\"; allow (read,search,write)" ACI_SUBJECT = " userdn = \"ldap:///%s\";)" % BIND_DN ACI_BODY = ACI_TARGET + ACI_ALLOW + ACI_SUBJECT # successful MOD with the ACI _bind_manager(topology_m2) suffix.add('aci', ACI_BODY) _bind_normal(topology_m2) request_ctrl = GetEffectiveRightsControl(criticality=True, authzId=ensure_bytes("dn: " + BIND_DN)) msg_id = topology_m2.ms["master1"].search_ext(PRODUCTION_DN, ldap.SCOPE_SUBTREE, "objectclass=*", serverctrls=[request_ctrl]) rtype, rdata, rmsgid, response_ctrl = topology_m2.ms["master1"].result3(msg_id) # ger={} value = '' for dn, attrs in rdata: topology_m2.ms["master1"].log.info("dn: %s" % dn) value = attrs['entryLevelRights'][0] topology_m2.ms["master1"].log.info("######## entryLevelRights: %r" % value) > assert b'n' in value E AssertionError: assert b'n' in b'v' suites/acl/acl_test.py:1049: AssertionError -------------------------------------------------------------------- Captured stderr call --------------------------------------------------------------------- INFO:lib389: ######## Disable the moddn aci mod ######## INFO:lib389:Bind as cn=Directory Manager INFO:lib389: ######## mode legacy : GER with moddn ######## INFO:lib389:Bind as cn=Directory Manager INFO:lib389:Bind as uid=bind_entry,dc=example,dc=com INFO:lib389:dn: cn=accounts,dc=example,dc=com INFO:lib389:dn: cn=excepts,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account0,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account1,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account3,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account5,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account6,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account7,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account8,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account9,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account10,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account11,cn=accounts,dc=example,dc=com INFO:lib389:dn: uid=new_account13,cn=accounts,dc=example,dc=com INFO:lib389:######## entryLevelRights: b'v' ======================================================== 1 failed, 22 passed, 2 error in 30.97 seconds ========================================================
<img alt="0001-Issue-lib389-3-Python-3-support-for-pwdPolicy_contro.patch" src="/lib389/issue/raw/files/d57761b96039e9f19dcad994f6100e4758436a07b35c49afe82f57073a1c50f4-0001-Issue-lib389-3-Python-3-support-for-pwdPolicy_contro.patch" />
Hey @amsharma This looks good. I would say one change is that for all the changes like:
(ldap.MOD_REPLACE, 'passwordMaxAge', b'5'),
Have a look at the lib389 file: https://pagure.io/lib389/blob/master/f/lib389/config.py
It's based on the DSLdapObject type so it does the byte saftey for you. You could change these lines to:
topo.standalone.config.set('passwordMaxAge', '5')
And the library will fix it to bytes for you. It's also a bit nicer to read than raw ldap mods.
Does that help?
Thanks @firstyear , I will do the changes accordingly. Thanks again!
<img alt="0001-Ticket-3-python-3-support-filter-test.patch" src="/lib389/issue/raw/files/299e6ab988da0a2bc8df701946613b3a4ebf7e97b3554a04c017241345a629a9-0001-Ticket-3-python-3-support-filter-test.patch" />
@firstyear ack
commit 5b6d1b26f63c2782d3ad7c376e0ecde382840735 To ssh://git@pagure.io/389-ds-base.git f4d86bb..6ef4eb5 master -> master
<img alt="0001-Issue-lib389-3-Python-3-support-for-pwdPolicy_contro.patch" src="/lib389/issue/raw/files/7364c11dbbbf9fd9b1c944bc809789982fee55731cee926326e1fb578a749520-0001-Issue-lib389-3-Python-3-support-for-pwdPolicy_contro.patch" />
Great! Thanks @amsharma it's a much nicer looking patch now. Ack from me.
commit e3d4a52eb7172dbbfaac2b4af9e4944ecf26482c To ssh://git@pagure.io/389-ds-base.git 6ef4eb5..e3d4a52 master -> master
Thanks for the commit @firstyear . Much appreciated!
Metadata Update from @spichugi: - Custom field Review Status adjusted to review (was: ack)
<img alt="0001-Issue-lib389-3-Python-3-support-for-ACL-test-suite.patch" src="/lib389/issue/raw/files/186dff9bd799a1dc3c32e8edcafc64db3f54bc3823fe9a6e686ffd5339b1a957-0001-Issue-lib389-3-Python-3-support-for-ACL-test-suite.patch" />
Metadata Update from @vashirov: - Custom field Review Status adjusted to ack (was: review)
commit fe859456a768ab21e896262947b164192f820e68 Author: Simon Pichugin spichugi@redhat.com Date: Tue Oct 17 15:50:23 2017 +0200
<img alt="0001-Ticket-lib389-3-python-3-support-for-betxn-test.patch" src="/lib389/issue/raw/files/b0ef985f9928e02663582e175ecc8ce6b61500e24a7685bd737baac3867e0e8f-0001-Ticket-lib389-3-python-3-support-for-betxn-test.patch" />
LGTM.
One small improvement will be nice to have though. We have docstrings for lib389/lib389/_mapped_object.py, so we need to have it for 'rename()' too. Just follow the example of docstrings of other methods.
<img alt="0001-Ticket-lib389-3-python-3-support-for-betxn-test.patch" src="/lib389/issue/raw/files/09a8d81275707951dceb72cdf3d4f2d297b81aa11ad4c52dc1b769017ef064c9-0001-Ticket-lib389-3-python-3-support-for-betxn-test.patch" />
Thanks mate. Here is the update with a docstring on rename.
https://fedorapeople.org/~spichugi/html/replica.html#lib389.replica.Replica.rename
Could you also specify parameters, please, as in example of other docstrings? With this kinda structure. It will make our generated docs looks good and consistent.
:param new_rdn: RDN of the new entry :type new_rdn: str :param newsuperior: New parent DN :type newsuperior: str
commit 9de5d88e200b1657f69c2c3f602f43f77cde0d60 To ssh://git@pagure.io/389-ds-base.git c0a7be7..9de5d88 master -> master
Added doc strings as mentioned :)
<img alt="0001-Ticket-3-lib389-config-test.patch" src="/lib389/issue/raw/files/69df7d4f3a729e560a5fb63072cc57d3be4c9644846adb7684a61af637dc7485-0001-Ticket-3-lib389-config-test.patch" />
Python 3 support for the config suite - adds LDBMConfig helper to wrap that, and simplifies the test suite considerably. Resolves an issue with multi-instance test cases and python setup/remove.
Metadata Update from @firstyear: - Custom field Review Status adjusted to review (was: ack)
<img alt="0001-Ticket-3-lib389-python-3-support-ds_logs-tests.patch" src="/lib389/issue/raw/files/a1dda5db91e1178d7de4f4761e1a005a0e362bd7f1e9528e7d89a4da1c681d0e-0001-Ticket-3-lib389-python-3-support-ds_logs-tests.patch" />
Another patch for review for ds_logs suite support. Fewer changes here to support this.
You have my ack for ds_logs test suite.
For the config test suite, I have only minor issues:
147 - log.info("Set nsslapd-maxbersize: 2097152 (default) to master2") 148 - try: 149 - topology_m2.ms["master2"].modify_s("cn=config", [(ldap.MOD_REPLACE, 150 - 'nsslapd-maxbersize', '2097152')]) 151 - except ldap.LDAPError as e: 152 - log.error('Failed to set nsslapd-maxbersize == 2097152 error ' + 153 - e.message['desc']) 154 - raise
You've removed the tearDown part, so next test case will nsslapd-maxbersize = 20480. Was it intentionally?
And could you please fix this:
330 + """ 331 + Manage "cn=config,cn=ldbm database,cn=plugins,cn=config" including: 332 + - Performance related tunings 333 + - DB backend settings 334 + """ 335 + def __init__(self, conn, batch=False): 336 + """@param conn - a DirSrv instance """ 337 + super(LDBMConfig, self).__init__(instance=conn, batch=batch) 338 + self._dn = DN_CONFIG_LDBM 339 + config_compare_exclude = [] 340 + self._rdn_attribute = 'cn'
To something like this:
"""Manage "cn=config,cn=ldbm database,cn=plugins,cn=config" including: - Performance related tunings - DB backend settings :param instance: An instance :type instance: lib389.DirSrv :param batch: Not implemented :type batch: bool """ def __init__(self, conn, batch=False): super(LDBMConfig, self).__init__(instance=conn, batch=batch) self._dn = DN_CONFIG_LDBM config_compare_exclude = [] self._rdn_attribute = 'cn'
Because the init part documentation is described in Class docstring (for Sphinx).
You have my ack for ds_logs test suite. For the config test suite, I have only minor issues: 147 - log.info("Set nsslapd-maxbersize: 2097152 (default) to master2") 148 - try: 149 - topology_m2.ms["master2"].modify_s("cn=config", [(ldap.MOD_REPLACE, 150 - 'nsslapd-maxbersize', '2097152')]) 151 - except ldap.LDAPError as e: 152 - log.error('Failed to set nsslapd-maxbersize == 2097152 error ' + 153 - e.message['desc']) 154 - raise
We never submit a packet larger that 2k, so it's no problem for this single test. Every other .py file will get a new instance with a clean bersize. Would only affect near by tests.
To something like this: """Manage "cn=config,cn=ldbm database,cn=plugins,cn=config" including: - Performance related tunings - DB backend settings :param instance: An instance :type instance: lib389.DirSrv :param batch: Not implemented :type batch: bool """ Because the init part documentation is described in Class docstring (for Sphinx).
To something like this: """Manage "cn=config,cn=ldbm database,cn=plugins,cn=config" including: - Performance related tunings - DB backend settings
:param instance: An instance :type instance: lib389.DirSrv :param batch: Not implemented :type batch: bool """
Done,
<img alt="0001-Ticket-3-lib389-config-test.patch" src="/lib389/issue/raw/files/563ef2b961e4d7f25a7bab540decf34e06425b1559ed6d97a4b41cca75709cf4-0001-Ticket-3-lib389-config-test.patch" />
commit 8ab0614e560a83b95dc7ccdfa591ea313d218d51 To ssh://git@pagure.io/389-ds-base.git 9de5d88..8ab0614 master -> master
To something like this: """Manage "cn=config,cn=ldbm database,cn=plugins,cn=config" including: - Performance related tunings - DB backend settings :param instance: An instance :type instance: lib389.DirSrv :param batch: Not implemented :type batch: bool """ Because the init part documentation is described in Class docstring (for Sphinx). Done,
Ok, also, please, remove line " 342 + """@param conn - a DirSrv instance """ ". And you have my ack. :)
commit 7f899a45f6d963a75df62481a9730b912ae325d3 To ssh://git@pagure.io/389-ds-base.git 8ab0614..7f899a4 master -> master
<img alt="0001-Ticket-lib389-Python-3-support-for-memberof-plugin-t.patch" src="/lib389/issue/raw/files/1311ab7378f555a12fafb39acc2c507155d0ef65d7751ec2d47bd8f5871b0964-0001-Ticket-lib389-Python-3-support-for-memberof-plugin-t.patch" />
Just be sure to "delete" the old ticket49064_test.py from dirsrvtests/tests/tickets/ too in the patch. You can "git commit" this if you rm that file, then git commit -a
I was helping with the patch, so I'm happy with the code, but @spichugi may have a comment to add :)
<img alt="0001-Ticket-3-lib389-python-3-compat-for-paged-results-te.patch" src="/lib389/issue/raw/files/7c318055f214209bbbc35698c0adfb29feae2309ce5923510f88a9f468052aee-0001-Ticket-3-lib389-python-3-compat-for-paged-results-te.patch" />
Here is a fun one for you @spichugi
<img alt="0001-Ticket-lib389-Python-3-support-for-memberof-plugin-t.patch" src="/lib389/issue/raw/files/0525f79310ed706dde9df3083ad2c35605e3f5439e24c1640bdb43d975f6e3ad-0001-Ticket-lib389-Python-3-support-for-memberof-plugin-t.patch" />
@firstyear Updated the patch. Thanks for pointing out and huge thanks for all your help.
@amsharma Perfect! Thank you. I'll leave @spichugi to comment also, he may have some specifics he wants checked,
@firstyear Sure. @spichugi Thanks for your tips too, please review the patch and provide your valuable feedback. Thanks again both of you :)
As we agreed before, we need to put ticket00000 files to the meaningful suites (and remove the number).
I think we can put the test case to .../suites/memberof_plugin/regression_test.py suite.
Also, you need to:
Next:
121 + 6. Step10 to Step 17 should pass with assertion error
It is not exactly what happens there. Assertion error is a test case code specific. Function _find_memberof checks that member is in the group or not. And we need to mention this information in the expectedresults.
Line '79 +', '89 +', '249 +' should be two blank lines.
I am mostly happy with the patch. Very good job!
One thing only... As it was discussed in IRC, we need to follow DS Schema so... Could you please change 'Organisation' to 'Organization' in the Class names and py module file name?
@spichugi Can I just add this as an alias to the schema instead ;)
I'll rename it,
<img alt="0001-Ticket-3-lib389-python-3-compat-for-paged-results-te.patch" src="/lib389/issue/raw/files/1ccecd6d433f42f9bc9495547d00027c73035743501861d905f0a8fd02a47202-0001-Ticket-3-lib389-python-3-compat-for-paged-results-te.patch" />
<img alt="0001-Ticket-lib389-Python-3-support-for-memberof-plugin-t.patch" src="/lib389/issue/raw/files/96e762d8f9b035f3605daf7b660ede3c4ed03ecdf26a8329bc3f5a39b59d0cce-0001-Ticket-lib389-Python-3-support-for-memberof-plugin-t.patch" />
commit f02cbddfa4a65e5ce2f6fbef1aadccacc922e0c5 Author: Amita Sharma amsharma@redhat.com Date: Thu Nov 9 18:41:05 2017 +0530
@firstyear you have my ack!
One small thing though. It still uses old notation. Patch lines:
1350 + super(Organisation, self).__init__(instance, dn, batch) 1362 + super(Organisations, self).__init__(instance, batch)
Also, I've missed one thing. Could you please add this to your classes?
class Organization(DSLdapObject): """A single instance of Organization entry :param instance: An instance :type instance: lib389.DirSrv :param dn: Entry DN :type dn: str :param batch: Not implemented :type batch: bool """ class Organizations(DSLdapObjects): """DSLdapObjects that represents Organizations entry :param instance: An instance :type instance: lib389.DirSrv :param basedn: Base DN for all group entries below :type basedn: str :param batch: Not implemented :type batch: bool """
Fixed :) commit 9e4b381530181025d8b198b05db13c6431155eae To ssh://git@pagure.io/389-ds-base.git 6ec27bc..9e4b381 master -> master
<img alt="0001-Ticket-3-lib389-python-3-support-for-subset-of-pwd-c.patch" src="/lib389/issue/raw/files/b5f52d295f8f44a027dee5db89e719340b1bfb4012aa89acce376f2be61a6c2b-0001-Ticket-3-lib389-python-3-support-for-subset-of-pwd-c.patch" />
Looks good to me. Only one small issue. Please, put a docstring with proper formatting (as other methods around) for:
def replace_many(self, *args):
<img alt="0005-Ticket-3-lib389-python-3-support-for-subset-of-pwd-c.patch" src="/lib389/issue/raw/files/ca42c85df9f12c1bacc484dce86ede205aee944084ec892454b1d80683cf71f6-0005-Ticket-3-lib389-python-3-support-for-subset-of-pwd-c.patch" />
Fixed
LGTM, ack!
commit 92209bf9746abc736118290d7ddaf846ee54d94a To ssh://git@pagure.io/389-ds-base.git 8a7bee6..92209bf master -> master
Login to comment on this ticket.