ipapython: simplify Env object initialization #266
Conversation
|
Only now I have noticed that this won't actually help fixing ticket 6482. Nevermind this PR then. |
|
I don't understand the "Nevermind this PR then.". fixing ticket 6482 is good but fixing ticket 6408 is required event more for @tiran work, right? |
|
From offline discussion I got that the PR should actually work in the end. I'll make the review. |
|
Yes, my above comment is wrong (sorry). |
|
This PR breaks almost all tests in test_ipalib/test_crud.py with |
Do not use custom Env instance to determine the debug level to use for the IPA API object - the IPA API object can properly determining the configured debug level on its own. https://fedorahosted.org/freeipa/ticket/6408
Add new API to initialize Env instances:
env = Env()
env.bootstrap(**overrides)
env.finalize()
This replaces the old private API:
env = Env()
env._bootstrap(**overrides)
env._finalize_core(**dict(DEFAULT_CONFIG))
env._finalize()
https://fedorahosted.org/freeipa/ticket/6408
| the instance is locked. After this method is called, no more | ||
| environment variables can be set during the remaining life of the | ||
| process. | ||
| 3. `Env.finalize()` - the instance is locked. After this method is |
There was a problem hiding this comment.
"the instance gets locked." is a better description of what happens here.
| if self.env_confdir is not None: | ||
| if self.env_confdir == self.confdir: | ||
| logger.info( | ||
| "IPA_CONFDIR env sets confdir to '%s'.", self.confdir) |
There was a problem hiding this comment.
At first I thought this part of code was completely useless but it turns out to appear later where it somehow made sense. Could you write a comment on why it's there? Also note that the check self.env_confdir == self.confdir: is not necessary as if env_confdir is set, confdir will always be equal to env_confdir.
| @@ -201,11 +202,9 @@ class provides high-level methods for bootstraping a fresh `Env` instance | |||
|
|
|||
| __locked = False | |||
|
|
|||
| def __init__(self, **initialize): | |||
| def __init__(self): | |||
There was a problem hiding this comment.
Is there a reason not to allow direct initialization? If so, please update the docstring of parameters.py:use_in_context() accordingly.
| @@ -498,42 +526,6 @@ def _bootstrap(self, **overrides): | |||
| self.plugins_on_demand = (self.context == 'cli') | |||
|
|
|||
| def _finalize_core(self, **defaults): | |||
| """ | |||
There was a problem hiding this comment.
Based on a meeting from last week it'd be better to add at least some kind of a brief docstring instead of removing it as a whole (even though it's been moved to bootstrap()
| """ | ||
| Initialize basic environment. | ||
| Complete initialization of standard IPA environment. |
There was a problem hiding this comment.
I'd rather go for a merge of the old and new: Initialize a standard IPA environment. It's still possible to add new values to the object so it's less confusing IMO.
| @@ -761,6 +754,8 @@ def finalize(self): | |||
|
|
|||
| self.__finalized = True | |||
|
|
|||
| self.env.finalize() | |||
There was a problem hiding this comment.
Was locking of the Env object missing previously or just done "ad-hoc" everywhere?
| env._bootstrap(context='installer', confdir=paths.ETC_IPA, log=None) | ||
| env._finalize_core(**dict(constants.DEFAULT_CONFIG)) | ||
| env.bootstrap(context='installer', confdir=paths.ETC_IPA, log=None) | ||
| env.finalize() |
There was a problem hiding this comment.
Locking the Env object seems to be a good idea, although it should probably be in a separate commit.
| @@ -389,7 +389,7 @@ def test_merge_from_file(self): | |||
| assert o._merge_from_file(override) == (4, 6) | |||
| for (k, v) in orig.items(): | |||
| assert o[k] is v | |||
| assert list(o) == sorted(keys + ('key0', 'key1', 'key2', 'key3', 'config_loaded')) | |||
| assert list(o) == sorted(keys + ('key0', 'key1', 'key2', 'key3')) | |||
There was a problem hiding this comment.
Why is 'config_loaded' not present among the keys? It should have been set during the last _merge_from_file(override) call.
|
@stlaz @HonzaCholasta any progress on this PR or should we mark it as postponed and return to it later? |
Fully initialize Env objects in Env() instead of having to call their
private methods to complete the initialization later.
Do not use custom Env instance to determine the debug level to use for the
IPA API object - the IPA API object can properly determining the
configured debug level on its own.
Remove locking and related code from Env as it is never used.
https://fedorahosted.org/freeipa/ticket/6408