#3 Python 3 compatibility
Opened 5 years ago by lbalhar. Modified 4 years ago

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

5 years ago

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)

4 years ago

Working on ACL suite now.

commit e8c56aaf2eadef1cc9aa6e8e39231ac9e31776c6
To ssh://git@pagure.io/389-ds-base.git
bc6dbf1..e8c56aa master -> master

@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 ========================================================

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!

commit 5b6d1b26f63c2782d3ad7c376e0ecde382840735
To ssh://git@pagure.io/389-ds-base.git
f4d86bb..6ef4eb5 master -> master

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)

4 years ago

Metadata Update from @vashirov:
- Custom field Review Status adjusted to ack (was: review)

4 years ago

commit fe859456a768ab21e896262947b164192f820e68
Author: Simon Pichugin spichugi@redhat.com
Date: Tue Oct 17 15:50:23 2017 +0200

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.

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

Metadata Update from @spichugi:
- Custom field Review Status adjusted to review (was: ack)

4 years ago

Metadata Update from @spichugi:
- Custom field Review Status adjusted to ack (was: review)

4 years ago

commit 9de5d88e200b1657f69c2c3f602f43f77cde0d60
To ssh://git@pagure.io/389-ds-base.git
c0a7be7..9de5d88 master -> master

Added doc strings as mentioned :)

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)

4 years ago

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).

Done,

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. :)

Metadata Update from @spichugi:
- Custom field Review Status adjusted to ack (was: review)

4 years ago

commit 7f899a45f6d963a75df62481a9730b912ae325d3
To ssh://git@pagure.io/389-ds-base.git
8ab0614..7f899a4 master -> master

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 :)

@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:

  • rename def test_ticket49064(topo) to something meaningful
  • put @pytest.mark.bz1352121 on the line before it
  • make the first line of the docstring more specific (what exactly we test there). Something like "Test that we allowed to enable MemberOf plugin in dedicated consumer"

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.

Here is a fun one for you @spichugi

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,

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

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):

commit 92209bf9746abc736118290d7ddaf846ee54d94a
To ssh://git@pagure.io/389-ds-base.git
8a7bee6..92209bf master -> master

Login to comment on this ticket.

Metadata
Attachments 19