On Tue, 2017-04-04 at 23:02 +, 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. 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]() 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
signature.asc
Description: This is a digitally signed message part
___
389-devel mailing list -- 389-devel@lists.fedoraproject.org
To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
