On Tue, 2017-04-04 at 23:02 +0000, Ankit Yadav wrote: > Hello Everyone, > > I was working on issue #1 lib389 cn=config comparison, I have done some work in that direction. I have added a compare function on DSLdapObject and have written a test for comparing user object. Currently this test is not complete, I am just printing the attributes values to verify the working of comparison function. > But comparison function is working, so let me know your feedback and how we can proceed further with this? > > Link to commit: https://pagure.io/fork/ankity10/lib389/c/44b02ba4ddb54be12041052b5b75f17b9eaf6b8f?branch=ticket-1-config-compare > Hey there, Looks like a great start. A lot of comments though, but thanks for all your effort and I hope these comments help you improve your code a lot. 27 + obj1_attrs_json = obj1.get_compare_attrs() 28 + obj2_attrs_json = obj2.get_compare_attrs() 29 + return DeepDiff(obj1_attrs_json, obj2_attrs_json) I don't think you should convert these to json. You should be comparing the dictionaries from the Entry directly. The JSON is only there for our future admin server to represent the entries in a website - it should never be used internally for operations in this library. JSON is also non-deterministic garbage IMO, so lets avoid it "unless we need it". You already have: 42 + attrs_entry = self._instance.getEntry(self._dn, ldap.SCOPE_BASE, "(objectclass=*)", ["*", "+"])42 + attrs_entry = self._instance.getEntry(self._dn, ldap.SCOPE_BASE, "(objectclass=*)", ["*", "+"]) So you should probably return either the entry data dict from here instead, then compare on that an example of this here: As "inspiration", look at: https://pagure.io/lib389/blob/master/f/lib389/_entry.py#_91 It's worth exploring and reading the code for what you use to find gems like this you may be able to copy or reuse. I think in this case, it's better to make a new compare function based on this code rather than trying to hack entry.__eq__ to work in this case, if that makes sense. 5 + from deepdiff import DeepDiff Please don't add library dependencies, unless it exists as a package on RHEL7, we can not support extra dependencies in our code. """ yum search deepdiff ... Warning: No matches found for: deepdiff No matches found """ With these lines: 12 class DSLdapObject(DSLogging): 13 + compare_exclude = [] 4 class UserAccount(DSLdapObject): 5 + compare_exclude = ['nsuniqueid'] 6 + Make sure you put these attributes in __init__() of the objects as: def __init__() self._compare_exclude = .... The reason for this is that the top level attribute is global to all instances. So if any subclass overrides the attribute, it causes the override of the parents attribute for ALL subclasses: this causes horrible horrible issues that are near impossible to resolve. You need to make these self.<attr> to indicate they are on the single instance only, which makes it work correctly. As well, because these are internal, prefix the variable with an _ to say it's private. Therefore: def __init__() self._compare_exclude = .... Remember, especially on the UserAccount, to put the self._compare_exclude *after* the call to super(UserAccount).__init__! , else the super call will just flush it back to the DSLdapObject value. With your test case, you don't need: 57 + assert (len(users.list()) == 1) Because if the user fails to create, we throw an exception from users.create, so the test will fail at that stage. :) 74 + print("testuser1 all attributes: ") try to use log.[info,debug,error](<message>) instead. This way based on the DEBBUGING environment variable we see more or less output. We shouldn't have "print" anywhere in the code base. 88 + # We want to capture std out while running this module through pytest, hence adding explicit test failure Run py.test with "-s" to show the stdout during the test instead :) Remember, to test users that are the same and also different! Also, a user compared to itself must be the same also, and a user compared to a different object class should also be false! IE compare a user to a group :) I hope that this advice helps you: I know it's a lot to change, but thanks for your work! -- Sincerely, William Brown Software Engineer Red Hat, Australia/Brisbane
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ 389-devel mailing list -- 389-devel@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to 389-devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
