I would like to work on this, I have installed 389-ds and forked the repo 23 days ago. I have been trying to understand the code. I have also read the execution flow of cli-tools created in the repository. I have also studied about LDAP. Need a your help in starting this because I am not sure how much code is done for this already.

So I would have a look at: lib389/config.py and lib389/_mapped_object.py.

Config derives from DSLdapObject, so I think that the best thing would be to write a compare function on DSLdapObject that just does a basic "compare two objects". You need to "ignore" a number of attributes like nsUniqueId, because this compare is checking if the attributes are the same.

If you have a look at DSLdapObject, I have a number of attributes like self._objectclasses = []
on it. You could make one called "self._compare_exclude", then have dsLdapObject's compare just use that attribute to exclude values. That way config you just list values you know will always be different, and you don't need to derive anything. You can work out the full set of attributes to compare from DSLdapObject with the schema query of the objectClasses

Does that help you?

This is also where the unit tests will help you. Instead of straight up writing the CLI tool, try writing a test case for this in lib389/tests/ first. You can have a look at the other tests for how to write these to setup an LDAP server, and you need to run the test as root (sorry! it needs root privs to setup ldap)

Then just write the generic, basicaly DSLdapObject compare first, and test comparing some simple groups or user accounts. You can find their definitions in lib389/idm/{group,user}.py

After that, we can get onto cn=config, comparing it, testing it, and then finally, the CLI tool to call it.

How does that sound?

If you get stuck running the tests or writing the code, feel free to email the 389-devel mailing list with any questions too.

Thanks ! This sounds great, I will first try to understand lib389/config.py and lib389/_mapped_object.py, then will try to write the compare function with its unit testing.

Awesome! I'm happy if you want to email in smaller patches and units of code so that we can help you out and make sure your on the right path. Don't be afraid to reach out, we really appreciate your enthusiasm and help.

Yes I will email you the code in small patches. This strategy will really help me. Thanks for appreciation.

Thanks for the patch. Really appreciate your time on this,

Few more things to comment on.

Can you remove the "+idea" from gitignore? I think this is local to your setup.

Don't import getsizeof, and please don't check the objects by size. There is a lot of complexity in python's vm, and the size is not a true test of equality. Let's be correct, not fast.

A better idea for this is:

 89 +        # Bail fast if the size doesn't match
 90 +        if getsizeof(obj1_attrs) != getsizeof(obj2_attrs):
 91 +            return False

if you want to see how many attributes were returned is to check len(attrs), because that will show you how many keys were there.

Of course, you check this in the next lines:

 93 +        if set(obj1_attrs.keys()) != set(obj2_attrs.keys()):
 94 +            return False

So just get rid of the size check.

Please remember to remove deepdiff from requirements.txt also.

Thanks again, I think it's getting much closer to complete!

Looks much better.

I'm going to apply the commit now. For future, just try to keep the commit message inside of about 100col width, and you may want to update your git email config.

Thanks for your hard work on this!

commit 861d0a22ea296cd6c55a598f31fa22bfc6e50b2e
To ssh://git@pagure.io/lib389.git
8f61549..de3d644 master -> master

So next is probably comparing across two instances.

You could create a test case that has two standalones, and then get a user from each and compare them. I think your current compare would work for this.

After that, it's a short step to comparing cn=config I think. That task is complicated by the way that some config parameters have the instance name in them, or some need ignoring.

I think that's probably the next two steps there.

Does that make sense?

Thanks again!

Yes, even I think that the current compare function should work for comparison of two RDN objects of different Directory Server instances. Yes, It make sense. For now, I will go for 1st step then for comparing cn=config.

Thanks for accepting the patch.

Hey @ankity10,

The patch looks great. Really good work on this and expanding those tests!

One tiny nitpick is that in

lib389/tests/idm/user_compare_m2Repl_test.py
+    if m1_ruv == m2_ruv:

In the else case where replication failed, there is no assert, so the test "does nothing".Couldn't we change this logic to:

assert(m1_ruv == m2_ruv)
m2_testuser = m2_users.get('testuser')
assert(UserAccount.compare(m1_testuser, m2_testuser) == True)
log.info("Test PASSED")

I think that this is simpler, and it guarantees replication did work, because in the current test if replication fails, the test doesn't test anything.

Thanks for your work on this!

Yeah, your suggestion is better than my current code. Thanks!

Ack, great work again with this one.

commit 813f9e8cfde0db62ff0063b2d4580e7aeaa22ab8
To ssh://git@pagure.io/lib389.git
2814d11..813f9e8 master -> master

Thanks, what next?

After merging the last one, I have the following test failure:

_______________________________________________________________________________________________ test_config_compare _______________________________________________________________________________________________
topology_i2 = <lib389.topologies.TopologyMain object at 0x7fd39008ea50>
    def test_config_compare(topology_i2):
        """
        Compare test between cn=config of two different Directory Server intance.
        """
        if DEBUGGING:
            # Add debugging steps(if any)...
            pass
        st1_config = topology_i2.ins.get('standalone1').config
        st2_config = topology_i2.ins.get('standalone2').config
        # 'nsslapd-port' attribute is expected to be same in cn=config comparison,
        # but they are different in our testing environment
        # as we are using 2 DS instances running, both running simultaneuosly.
        # Hence explicitly adding 'nsslapd-port' to compare_exclude.
        st1_config._compare_exclude.append('nsslapd-port')
        st2_config._compare_exclude.append('nsslapd-port')
>       assert(Config.compare(st1_config, st2_config) == True)
lib389/tests/config_compare_test.py:48: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
lib389/_mapped_object.py:249: in compare
    if obj1.rdn != obj2.rdn:
lib389/_mapped_object.py:152: in rdn
    return ensure_str(self.get_attr_val(self._rdn_attribute))
lib389/_mapped_object.py:319: in get_attr_val
    entry = self._instance.search_s(self._dn, ldap.SCOPE_BASE, attrlist=[key])[0]
lib389/__init__.py:161: in inner
    return f(*args, **kwargs)
/usr/lib64/python2.7/site-packages/ldap/ldapobject.py:597: in search_s
    return self.search_ext_s(base,scope,filterstr,attrlist,attrsonly,None,None,timeout=self.timeout)
lib389/__init__.py:161: in inner
    return f(*args, **kwargs)
/usr/lib64/python2.7/site-packages/ldap/ldapobject.py:590: in search_ext_s
    msgid = self.search_ext(base,scope,filterstr,attrlist,attrsonly,serverctrls,clientctrls,timeout,sizelimit)
lib389/__init__.py:161: in inner
    return f(*args, **kwargs)
/usr/lib64/python2.7/site-packages/ldap/ldapobject.py:586: in search_ext
    timeout,sizelimit,
lib389/__init__.py:161: in inner
    return f(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <lib389.DirSrv object at 0x7fd390319790>, func = <built-in method search_ext of LDAP object at 0x7fd3902f7c60>, args = ('cn=config', 0, '(objectClass=*)', [None], 0, None, ...), 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         TypeError: ('expected string in list', None)
/usr/lib64/python2.7/site-packages/ldap/ldapobject.py:106: TypeError

Hi @ilias95, Thanks for reporting the issue.
The reason I did not see this issue in my local setup because my working branch is behind the master branch.

I have updated my working repository, I will soon fix the issue.

@ankity10 @ilias95 The issue was introduced in #26, I've put a fix there.

Actually, hold that though. The issue is the missing _rdn_attribute on config.py Config.

@ankity10 @ilias95 Can you review the patch above please :)

I had applied your patch to my local setup and It had solved the issue reported by @ilias95 . Also the patch looks good.

Thanks so much for looking at this!

@spichugi would you mind having a look and acking this?

@firstyear Tbh, I didn't follow the discussion on this, but this patch indeed solves the problem for me. Thanks!

commit dfb3bb01a13ce446430ce1ee8f014d0333ca3c08
To ssh://git@pagure.io/lib389.git
33bb816..d0fbf04 master -> master

Hey mate. Sorry for long delay, I've been busy at a training.

_generic_compare should say "first object" not first user, as the generics can be used for any object type :)

Otherwise I think the patch looks good. I would like to apply it and try it. Perhaps if we are doing compare maybe we need to go back and do a proper diff rather than just true/false compare. I feel bad as I think I told you do to true/false first but now I see we need a diff ....

Hope that helps, again really great work. loving the methodical work you are doing here.

Hi @firstyear, Thanks! for the review. I was busy in exams. But I will do the changes as soon as possible.
Now regarding the diff, even I thought of a diff at the time of writing "compare" but anyways change is a part of progress. We can still do a diff.
we can talk about the "diff" in coming week.
Thanks!

Yep, look forward to hearing from you about it. Hope exams went well mate.

This is how the diff works right now => https://paste.fedoraproject.org/paste/F5Uxj~1pF8~BGiYbhwC0TF5M1UNdIGYhyRLivL9gydE=

Hey.

I've got a question. Why did you declare _diff() and display_diff() as class methods? I think that class methods are supposed to return new instances of the class normally. If you just don't need access to instance variables you can declare the methods as static instead.

Also, I think that since display_diff() is responsible for displaying results / printing, it should be at a higher level, under lib389/cli_idm/ I guess.

Thanks.

Great review @ilias95, these are all comments that I agree with.

@ankity10 I think that display_diff as @ilias95 said, should be part of the cli tool - we should pass back the tuple to the caller and it works out to present it in a meaningful manner,

Otherwise, I'm really impressed by this, it's a really good change. It would be good to expand the tests cases to show the diff works "as expected" too.

I agree with @ilias95 I should change that function to "static" instead of "classmethod". I will also shift display_diff to cli tools.

Thanks! for the review @ilias95 and @firstyear.

@ankity10 mate, I think you have submitted the same patch again by mistake.

Thanks mate!

This is how diff for different user looks like : https://paste.fedoraproject.org/paste/uoFVTx1U1qtV~IX64oc~xQ

this is how diff for same user looks like: https://paste.fedoraproject.org/paste/S8XTXCxFWgDxfp~Poj8z2g

The patch is looking really good! Well done.

I have two more questions. Can we expand the tests for comparison with this, IE so we see expected results from the diff and assert it's correct?

As well in that test case, it would be good to see if you handle compare (user, group) properly. :)

I think once you expand the tests, this is ready for merge. :)

Yes, We can surely extend tests for compare as well as handle the user, group comparison.

@ankity10 Would you like to make the tests part of this patch? or a follow up?

@firstyear I would like to make it as a follow up patch.

Okay, when you submit the second patch for the tests, I'll merge them both at the same time. Is that okay?

Yeah, that's fine.