Check for conflict entries before raising domain level #324
Conversation
| return True | ||
| else: | ||
| return False | ||
|
|
There was a problem hiding this comment.
I would rather rewrite the code to raise exception directly from this function:
+ try:
+ ldap.get_entries(
+ filter="(&(nsds5replconflict=*)(|(objectclass=ldapsubentry)(objectclass=*)))",
+ base_dn=container_dn,
+ scope=ldap.SCOPE_SUBTREE)
+
+ raise errors.InvalidDomainLevelError(...)
+ except errors.NotFound:
+ return
I use get_entreis here because it raises NotFound when no conflicts are found.
| @@ -131,6 +155,13 @@ def execute(self, *args, **options): | |||
| .format(desired_value, master['cn'][0])) | |||
| raise errors.InvalidDomainLevelError(reason=message) | |||
|
|
|||
| # Check if conflict entries exist in topology subtree, should be resolved first | |||
| if check_conflict_entries(ldap, self.api): | |||
There was a problem hiding this comment.
You can then just plainly call check_conflict_entries(ldap, desired_value, self.api) here.
|
I have some comments regarding the logic of the check. These will also fix the warning reported by pylint checker. |
| filter="(&(nsds5replconflict=*)(|(objectclass=ldapsubentry)(objectclass=*)))", | ||
| base_dn=container_dn, | ||
| scope=ldap.SCOPE_SUBTREE) | ||
| return True |
There was a problem hiding this comment.
Please raise the InvalidDomainLevel here directly, there is no point in returning a boolean and then checking it in the caller to raise exception.
This leads to cleaner code uncluttered with extraneous boolean checks.
|
I am still not happy with the the PR, see review comments. |
Checking of conflicts is not only done in topology container as tests showed it can occurs elsewhere https://fedorahosted.org/freeipa/ticket/6534
|
There were some PEP8 errors but I fixed them and pushed to: ipa-4-4: |
Checking of conflicts is not only done in topology container as
tests showed it can occurs elsewhere
https://fedorahosted.org/freeipa/ticket/6534