Break ipaplatform / ipalib import cycle of hell #258
Conversation
| self.api = api | ||
| else: | ||
| import ipalib # FixMe: break import cycle | ||
| self.api = ipalib.api |
There was a problem hiding this comment.
This import is for backwards compatibility with external code. It can be removed in a future version of FreeIPA.
There was a problem hiding this comment.
Maybe would be better to make this comment to code, I'm not sure if we should have else branch there. We never declared this as a public stable API.
There was a problem hiding this comment.
The else block is required to keep b/w compatibility with scripts like copy-schema-to-ca.py.
| services = dict() | ||
| for s in base_services.wellknownservices: | ||
| services[s] = self.service_class_factory(s) | ||
| services[s] = self.service_class_factory(s, ipalib.api) |
There was a problem hiding this comment.
I'm not proud of this fix. Do we need a list of service instances at all? Or can we get rid of it and use services.service() everywhere?
| @@ -104,7 +104,7 @@ def restart_pki_ds(): | |||
| """Restart the CA DS instance to pick up schema changes | |||
| """ | |||
| root_logger.info('Restarting CA DS') | |||
| services.service('dirsrv').restart(SERVERID) | |||
| services.service('dirsrv', api).restart(SERVERID) | |||
There was a problem hiding this comment.
This is really special code, according docs, this code must be copied from RHEL7 to RHEL6 and then executed on RHEL6. S this change will break RHEL6. IMO this code should be set to stone as is :)
Here is an attempt to break the import cycle of hell between ipaplatform and ipalib. All services now pass an ipalib.api object to services.service(). RedHatServices.__init__() still needs to do a local import because it initializes its wellknown service dict with service instances. Signed-off-by: Christian Heimes <cheimes@redhat.com>
tiran
force-pushed
the
break_import_cycles
branch
from
f89393b
to
1896b0d
Compare
|
thx @mbasti-rh . I took care of it. |
|
LGTM, except the inline comment I made, I'll test it tomorrow |
|
The original code is broken by design IMO. The API object is used only to get the configured service startup timeout and to guess our DS instance name. None of this is platform specific, so I would prefer if we removed this from Anyway, given that the current plan is to make |
|
It's no longer a priority, but I still like to fix the imports No matter what the cyclic imports and cross-package dependencies are a mess. A wrong order of imports can trigger an import error. Even with this PR, ipapython.dogtag triggers an import of ipalib.api. In the long run neither ipaplatform nor ipapython should have an import dependency on ipalib or a global ipalib.api object. Instead the API object should be passed. |
|
ACK from me if @jcholast is not against it |
|
I'm OK with it. |
|
Fixed upstream |
Here is an attempt to break the import cycle of hell between ipaplatform
and ipalib. All services now pass an ipalib.api object to
services.service(). RedHatServices.init() still needs to do a local
import because it initializes its wellknown service dict with service
instances.
Signed-off-by: Christian Heimes cheimes@redhat.com