URL: https://github.com/freeipa/freeipa/pull/266 Author: jcholast Title: #266: ipapython: simplify Env object initialization Action: opened
PR body: """ 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 """ To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/266/head:pr266 git checkout pr266
From 1f3bfa0bd13af4e8cba51b8149c397ceeccdcf14 Mon Sep 17 00:00:00 2001 From: Jan Cholasta <jchol...@redhat.com> Date: Tue, 22 Nov 2016 12:42:40 +0100 Subject: [PATCH 1/3] wsgi, oddjob: remove needless uses of Env 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 --- doc/guide/wsgi.py.txt | 12 +----------- install/oddjob/com.redhat.idm.trust-fetch-domains | 9 +-------- install/share/wsgi.py | 12 +----------- 3 files changed, 3 insertions(+), 30 deletions(-) diff --git a/doc/guide/wsgi.py.txt b/doc/guide/wsgi.py.txt index 8566a25..71d7a2b 100644 --- a/doc/guide/wsgi.py.txt +++ b/doc/guide/wsgi.py.txt @@ -1,16 +1,6 @@ from ipalib import api -from ipalib.config import Env -from ipalib.constants import DEFAULT_CONFIG -# Determine what debug level is configured. We can only do this -# by reading in the configuration file(s). The server always reads -# default.conf and will also read in `context'.conf. -env = Env() -env._bootstrap(context='server', log=None) -env._finalize_core(**dict(DEFAULT_CONFIG)) - -# Initialize the API with the proper debug level -api.bootstrap(context='server', debug=env.debug, log=None) (ref:wsgi-app-bootstrap) +api.bootstrap(context='server', log=None) (ref:wsgi-app-bootstrap) try: api.finalize() (ref:wsgi-app-finalize) except Exception as e: diff --git a/install/oddjob/com.redhat.idm.trust-fetch-domains b/install/oddjob/com.redhat.idm.trust-fetch-domains index b663daa..e053474 100755 --- a/install/oddjob/com.redhat.idm.trust-fetch-domains +++ b/install/oddjob/com.redhat.idm.trust-fetch-domains @@ -5,8 +5,6 @@ from ipaserver.install.installutils import is_ipa_configured, ScriptError from ipapython import config, ipautil from ipalib import api from ipapython.dn import DN -from ipalib.config import Env -from ipalib.constants import DEFAULT_CONFIG from ipapython.ipautil import kinit_keytab from ipaplatform.constants import constants import sys @@ -89,12 +87,7 @@ if len(args) != 1: trusted_domain = ipautil.fsdecode(args[0]).lower() -env = Env() -env._bootstrap(debug=options.debug, log=None) -env._finalize_core(**dict(DEFAULT_CONFIG)) - -# Initialize the API with the proper debug level -api.bootstrap(in_server=True, debug=env.debug, log=None, context='server') +api.bootstrap(in_server=True, debug=options.debug, log=None, context='server') api.finalize() # Only import trust plugin after api is initialized or internal imports diff --git a/install/share/wsgi.py b/install/share/wsgi.py index ee9311e..88316f6 100644 --- a/install/share/wsgi.py +++ b/install/share/wsgi.py @@ -24,18 +24,8 @@ WSGI appliction for IPA server. """ from ipalib import api -from ipalib.config import Env -from ipalib.constants import DEFAULT_CONFIG -# Determine what debug level is configured. We can only do this -# by reading in the configuration file(s). The server always reads -# default.conf and will also read in `context'.conf. -env = Env() -env._bootstrap(context='server', log=None) -env._finalize_core(**dict(DEFAULT_CONFIG)) - -# Initialize the API with the proper debug level -api.bootstrap(context='server', debug=env.debug, log=None) +api.bootstrap(context='server', log=None) try: api.finalize() except Exception as e: From 5c9290bcc6dda2dc8582f45e9cf6baaafa663309 Mon Sep 17 00:00:00 2001 From: Jan Cholasta <jchol...@redhat.com> Date: Tue, 22 Nov 2016 12:46:17 +0100 Subject: [PATCH 2/3] ipapython: simplify Env object initialization Fully initialize Env objects in Env() instead of having to call their private methods to complete the initialization later. https://fedorahosted.org/freeipa/ticket/6408 --- install/tools/ipa-pki-retrieve-key | 1 - ipaclient/install/client.py | 5 +++++ ipalib/config.py | 6 ++--- ipalib/plugable.py | 5 +---- ipaserver/install/server/replicainstall.py | 4 +--- ipatests/test_ipalib/test_config.py | 35 ++++++++++++++++++------------ 6 files changed, 31 insertions(+), 25 deletions(-) diff --git a/install/tools/ipa-pki-retrieve-key b/install/tools/ipa-pki-retrieve-key index 740e799..f1a4195 100755 --- a/install/tools/ipa-pki-retrieve-key +++ b/install/tools/ipa-pki-retrieve-key @@ -11,7 +11,6 @@ from ipaplatform.paths import paths from ipapython.secrets.client import CustodiaClient env = Env() -env._finalize() keyname = "ca_wrapped/" + sys.argv[1] servername = sys.argv[2] diff --git a/ipaclient/install/client.py b/ipaclient/install/client.py index b24a989..757377b 100644 --- a/ipaclient/install/client.py +++ b/ipaclient/install/client.py @@ -2937,6 +2937,11 @@ def uninstall_check(options): "IPA client is not configured on this system.", rval=CLIENT_NOT_CONFIGURED) + # We will need at least api.env, finalize api now. This system is + # already installed, so the configuration file is there. + api.bootstrap(context='cli_installer', debug=options.debug) + api.finalize() + server_fstore = sysrestore.FileStore(paths.SYSRESTORE) if server_fstore.has_files() and not options.on_master: root_logger.error( diff --git a/ipalib/config.py b/ipalib/config.py index 1075d62..c42ab90 100644 --- a/ipalib/config.py +++ b/ipalib/config.py @@ -41,7 +41,7 @@ from ipapython.dn import DN from ipalib.base import check_name -from ipalib.constants import CONFIG_SECTION +from ipalib.constants import CONFIG_SECTION, DEFAULT_CONFIG from ipalib.constants import OVERRIDE_ERROR, SET_ERROR, DEL_ERROR if six.PY3: @@ -203,8 +203,8 @@ class provides high-level methods for bootstraping a fresh `Env` instance def __init__(self, **initialize): object.__setattr__(self, '_Env__d', {}) object.__setattr__(self, '_Env__done', set()) - if initialize: - self._merge(**initialize) + self._bootstrap(**initialize) + self._finalize_core(**dict(DEFAULT_CONFIG)) def __lock__(self): """ diff --git a/ipalib/plugable.py b/ipalib/plugable.py index 76fb9fd..586784f 100644 --- a/ipalib/plugable.py +++ b/ipalib/plugable.py @@ -43,7 +43,6 @@ from ipalib.text import _ from ipalib.util import classproperty from ipalib.base import ReadOnly, lock, islocked -from ipalib.constants import DEFAULT_CONFIG from ipapython import ipautil from ipapython.ipa_log_manager import ( log_mgr, @@ -350,7 +349,6 @@ def __init__(self): self.__instances = {} self.__next = {} self.__done = set() - self.env = Env() @property def bases(self): @@ -433,8 +431,7 @@ def bootstrap(self, parser=None, **overrides): self.log_mgr = log_mgr log = log_mgr.root_logger self.log = log - self.env._bootstrap(**overrides) - self.env._finalize_core(**dict(DEFAULT_CONFIG)) + self.env = Env(**overrides) # Add the argument parser if not parser: diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py index a7b333c..8904e39 100644 --- a/ipaserver/install/server/replicainstall.py +++ b/ipaserver/install/server/replicainstall.py @@ -951,9 +951,7 @@ def promote_check(installer): fstore = sysrestore.FileStore(paths.SYSRESTORE) - env = Env() - env._bootstrap(context='installer', log=None) - env._finalize_core(**dict(constants.DEFAULT_CONFIG)) + env = Env(context='installer', log=None) # pylint: disable=no-member xmlrpc_uri = 'https://{}/ipa/xml'.format(ipautil.format_netloc(env.host)) diff --git a/ipatests/test_ipalib/test_config.py b/ipatests/test_ipalib/test_config.py index 22373e1..f9656c9 100644 --- a/ipatests/test_ipalib/test_config.py +++ b/ipatests/test_ipalib/test_config.py @@ -27,7 +27,7 @@ from ipatests.util import raises, delitem, ClassChecker from ipatests.util import getitem from ipatests.util import TempDir, TempHome -from ipalib.constants import OVERRIDE_ERROR, SET_ERROR, DEL_ERROR +from ipalib.constants import OVERRIDE_ERROR, DEL_ERROR from ipalib.constants import NAME_REGEX, NAME_ERROR from ipalib import config, constants, base from ipaplatform.paths import paths @@ -155,9 +155,10 @@ def test_init(self): Test the `ipalib.config.Env.__init__` method. """ o = self.cls() - assert list(o) == [] - assert len(o) == 0 - assert o.__islocked__() is False + default_keys = [k for k, _v in constants.DEFAULT_CONFIG] + assert list(o) == sorted(default_keys + ['config_loaded']) + assert len(o) == len(default_keys) + 1 + assert o.__islocked__() is True def test_lock(self): """ @@ -298,13 +299,13 @@ def test_len(self): Test the `ipalib.config.Env.__len__` method. """ o = self.cls() - assert len(o) == 0 + default_len = len(o) for i in range(1, 11): key = 'key%d' % i value = u'value %d' % i o[key] = value assert o[key] is value - assert len(o) == i + assert len(o) == default_len + i def test_iter(self): """ @@ -334,24 +335,27 @@ def test_merge(self): ('key5', u'Value 5'), ) o = self.cls() + default_len = len(o) + default_keys = list(o) assert o._merge(**dict(group1)) == (4, 4) - assert len(o) == 4 - assert list(o) == list(key for (key, value) in group1) + assert len(o) == default_len + 4 + assert list(o) == ( + sorted(default_keys + list(key for (key, value) in group1))) for (key, value) in group1: assert getattr(o, key) is value assert o[key] is value assert o._merge(**dict(group2)) == (2, 4) - assert len(o) == 6 + assert len(o) == default_len + 6 expected = dict(group2) expected.update(dict(group1)) - assert list(o) == sorted(expected) + assert list(o) == sorted(default_keys + list(expected)) assert expected['key2'] == 'value 2' # And not 'Value 2' for (key, value) in expected.items(): assert getattr(o, key) is value assert o[key] is value assert o._merge(**expected) == (0, 6) - assert len(o) == 6 - assert list(o) == sorted(expected) + assert len(o) == default_len + 6 + assert list(o) == sorted(default_keys + list(expected)) def test_merge_from_file(self): """ @@ -364,7 +368,6 @@ def test_merge_from_file(self): no_exist = tmp.join('no_exist.conf') assert not path.exists(no_exist) o = self.cls() - o._bootstrap() keys = tuple(o) orig = dict((k, o[k]) for k in o) assert o._merge_from_file(no_exist) is None @@ -390,7 +393,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')) for i in range(4): assert o['key%d' % i] == ('var%d' % i) keys = tuple(o) @@ -418,6 +421,10 @@ def new(self, in_tree=False): """ home = TempHome() o = self.cls() + for k in o: + object.__delattr__(o, k) + object.__setattr__(o, '_Env__d', {}) + object.__setattr__(o, '_Env__done', set()) if in_tree: o.in_tree = True return (o, home) From 104e9ec1c9d2ae5196976b457e5f228631d7667b Mon Sep 17 00:00:00 2001 From: Jan Cholasta <jchol...@redhat.com> Date: Tue, 22 Nov 2016 12:52:26 +0100 Subject: [PATCH 3/3] ipapython: remove unused code from Env Remove locking and related code from Env as it is never used. https://fedorahosted.org/freeipa/ticket/6408 --- ipalib/__init__.py | 2 +- ipalib/config.py | 75 +------------------------------- ipatests/test_ipalib/test_config.py | 86 +------------------------------------ 3 files changed, 3 insertions(+), 160 deletions(-) diff --git a/ipalib/__init__.py b/ipalib/__init__.py index 4a61ace..63cc342 100644 --- a/ipalib/__init__.py +++ b/ipalib/__init__.py @@ -733,7 +733,7 @@ class my_command(Command): in `ipalib.constants.DEFAULT_CONFIG`. Also, you should consider whether your new environment variables should have any auto-magic logic to determine their values if they haven't already been set by the time `config.Env._bootstrap()`, -`config.Env._finalize_core()`, or `config.Env._finalize()` is called. +`config.Env._finalize_core()` is called. On the other hand, if your plugin requires new environment variables and will be installed in a 3rd-party package, your plugin should set these variables diff --git a/ipalib/config.py b/ipalib/config.py index c42ab90..26daeb9 100644 --- a/ipalib/config.py +++ b/ipalib/config.py @@ -21,11 +21,6 @@ """ Process-wide static configuration and environment. -The standard run-time instance of the `Env` class is initialized early in the -`ipalib` process and is then locked into a read-only state, after which no -further changes can be made to the environment throughout the remaining life -of the process. - For the per-request thread-local information, see `ipalib.request`. """ @@ -42,7 +37,7 @@ from ipapython.dn import DN from ipalib.base import check_name from ipalib.constants import CONFIG_SECTION, DEFAULT_CONFIG -from ipalib.constants import OVERRIDE_ERROR, SET_ERROR, DEL_ERROR +from ipalib.constants import OVERRIDE_ERROR, DEL_ERROR if six.PY3: unicode = str @@ -144,18 +139,6 @@ class Env(object): ... AttributeError: cannot override Env.date value u'First' with 'Second' - An `Env` instance can be *locked*, after which no further variables can be - set. Trying to set variables on a locked `Env` instance will also raise - an ``AttributeError``. For example: - - >>> env = Env() - >>> env.okay = 'This will work.' - >>> env.__lock__() - >>> env.nope = 'This wont work!' - Traceback (most recent call last): - ... - AttributeError: locked: cannot set Env.nope to 'This wont work!' - `Env` instances also provide standard container emulation for membership testing, counting, and iteration. For example: @@ -188,40 +171,17 @@ class provides high-level methods for bootstraping a fresh `Env` instance third-party plugins can merge-in defaults for additional variables they use (likely using the `Env._merge()` method). - 3. `Env._finalize()` - one last chance to merge-in variables and then - the instance is locked. After this method is called, no more - environment variables can be set during the remaining life of the - process. - However, normally none of these three bootstraping methods are called directly and instead only `plugable.API.bootstrap()` is called, which itself takes care of correctly calling the `Env` bootstrapping methods. """ - __locked = False - def __init__(self, **initialize): object.__setattr__(self, '_Env__d', {}) object.__setattr__(self, '_Env__done', set()) self._bootstrap(**initialize) self._finalize_core(**dict(DEFAULT_CONFIG)) - def __lock__(self): - """ - Prevent further changes to environment. - """ - if self.__locked is True: - raise Exception( - '%s.__lock__() already called' % self.__class__.__name__ - ) - object.__setattr__(self, '_Env__locked', True) - - def __islocked__(self): - """ - Return ``True`` if locked. - """ - return self.__locked - def __setattr__(self, name, value): """ Set the attribute named ``name`` to ``value``. @@ -234,10 +194,6 @@ def __setitem__(self, key, value): """ Set ``key`` to ``value``. """ - if self.__locked: - raise AttributeError( - SET_ERROR % (self.__class__.__name__, key, value) - ) check_name(key) if key in self.__d: raise AttributeError(OVERRIDE_ERROR % @@ -511,8 +467,6 @@ def _finalize_core(self, **defaults): so that 3rd-party plugins can set variables they may require as the plugins are registered. - Also see `Env._finalize()`, the final method in the bootstrap sequence. - :param defaults: Internal defaults for all built-in variables. """ self.__doing('_finalize_core') @@ -561,30 +515,3 @@ def _finalize_core(self, **defaults): self.server = parsed.netloc self._merge(**defaults) - - def _finalize(self, **lastchance): - """ - Finalize and lock environment. - - This method will perform the following steps: - - 1. Call `Env._finalize_core()` if it hasn't already been called. - - 2. Merge-in the variables in ``lastchance`` by calling - `Env._merge()`. - - 3. Lock this `Env` instance, after which no more environment - variables can be set on this instance. Aside from unit-tests - and example code, normally only one `Env` instance is created, - which means that after this step, no more variables can be set - during the remaining life of the process. - - This method should be called after all plugins have been loaded and - after `plugable.API.finalize()` has been called. - - :param lastchance: Any final variables to merge-in before locking. - """ - self.__doing('_finalize') - self.__do_if_not_done('_finalize_core') - self._merge(**lastchance) - self.__lock__() diff --git a/ipatests/test_ipalib/test_config.py b/ipatests/test_ipalib/test_config.py index f9656c9..5577040 100644 --- a/ipatests/test_ipalib/test_config.py +++ b/ipatests/test_ipalib/test_config.py @@ -29,7 +29,7 @@ from ipatests.util import TempDir, TempHome from ipalib.constants import OVERRIDE_ERROR, DEL_ERROR from ipalib.constants import NAME_REGEX, NAME_ERROR -from ipalib import config, constants, base +from ipalib import config, constants from ipaplatform.paths import paths import pytest @@ -158,37 +158,6 @@ def test_init(self): default_keys = [k for k, _v in constants.DEFAULT_CONFIG] assert list(o) == sorted(default_keys + ['config_loaded']) assert len(o) == len(default_keys) + 1 - assert o.__islocked__() is True - - def test_lock(self): - """ - Test the `ipalib.config.Env.__lock__` method. - """ - o = self.cls() - assert o.__islocked__() is False - o.__lock__() - assert o.__islocked__() is True - e = raises(Exception, o.__lock__) - assert str(e) == 'Env.__lock__() already called' - - # Also test with base.lock() function: - o = self.cls() - assert o.__islocked__() is False - assert base.lock(o) is o - assert o.__islocked__() is True - e = raises(AssertionError, base.lock, o) - assert str(e) == 'already locked: %r' % o - - def test_islocked(self): - """ - Test the `ipalib.config.Env.__islocked__` method. - """ - o = self.cls() - assert o.__islocked__() is False - assert base.islocked(o) is False - o.__lock__() - assert o.__islocked__() is True - assert base.islocked(o) is True def test_setattr(self): """ @@ -207,13 +176,6 @@ def test_setattr(self): e = raises(AttributeError, setattr, o, name, raw) assert str(e) == OVERRIDE_ERROR % ('Env', name, value, raw) - # Test that values cannot be set once locked: - o = self.cls() - o.__lock__() - for (name, raw, value) in good_vars: - e = raises(AttributeError, setattr, o, name, raw) - assert str(e) == SET_ERROR % ('Env', name, raw) - # Test that name is tested with check_name(): o = self.cls() for (name, value) in bad_names: @@ -237,13 +199,6 @@ def test_setitem(self): e = raises(AttributeError, o.__setitem__, key, raw) assert str(e) == OVERRIDE_ERROR % ('Env', key, value, raw) - # Test that values cannot be set once locked: - o = self.cls() - o.__lock__() - for (key, raw, value) in good_vars: - e = raises(AttributeError, o.__setitem__, key, raw) - assert str(e) == SET_ERROR % ('Env', key, raw) - # Test that name is tested with check_name(): o = self.cls() for (key, value) in bad_names: @@ -509,13 +464,11 @@ def finalize_core(self, ctx, **defaults): set_here = ('in_server', 'logdir', 'log') assert o._isdone('_bootstrap') is False assert o._isdone('_finalize_core') is False - assert o._isdone('_finalize') is False for key in set_here: assert key not in o o._finalize_core(**defaults) assert o._isdone('_bootstrap') is True assert o._isdone('_finalize_core') is True - assert o._isdone('_finalize') is False # Should not cascade for key in set_here: assert key in o @@ -578,40 +531,3 @@ def test_finalize_core(self): if key == 'mode': continue assert o[key] == value, '%r is %r; should be %r' % (key, o[key], value) - - def test_finalize(self): - """ - Test the `ipalib.config.Env._finalize` method. - """ - # Check that calls cascade up the chain: - o, _home = self.new(in_tree=True) - assert o._isdone('_bootstrap') is False - assert o._isdone('_finalize_core') is False - assert o._isdone('_finalize') is False - o._finalize() - assert o._isdone('_bootstrap') is True - assert o._isdone('_finalize_core') is True - assert o._isdone('_finalize') is True - - # Check that it can't be called twice: - e = raises(Exception, o._finalize) - assert str(e) == 'Env._finalize() already called' - - # Check that _finalize() calls __lock__() - o, _home = self.new(in_tree=True) - assert o.__islocked__() is False - o._finalize() - assert o.__islocked__() is True - e = raises(Exception, o.__lock__) - assert str(e) == 'Env.__lock__() already called' - - # Check that **lastchance works - o, _home = self.finalize_core(None) - key = 'just_one_more_key' - value = u'with one more value' - lastchance = {key: value} - assert key not in o - assert o._isdone('_finalize') is False - o._finalize(**lastchance) - assert key in o - assert o[key] is value
-- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
