From abb964a5a0f03e926bc01d999be332f60ff6194a Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Wed, 30 Jan 2019 16:25:36 -0500 Subject: [PATCH] move extravars and option vars loading into VM (#51070) * move extravars and option vars loading into VM also safedir setting, all these are intrinsic to VM avoid uneeded and inefectual shallow copy remove setters/getters as VM now does most of the work in init updated and added tests * feedback + fixes * keep extra_vars property for vars_prompt * pass values not objects --- changelogs/fragments/vm_updates.yml | 2 ++ lib/ansible/cli/__init__.py | 15 +-------- lib/ansible/utils/vars.py | 2 ++ lib/ansible/vars/manager.py | 43 +++++++++--------------- test/units/vars/test_variable_manager.py | 24 +++++++++---- 5 files changed, 39 insertions(+), 47 deletions(-) create mode 100644 changelogs/fragments/vm_updates.yml diff --git a/changelogs/fragments/vm_updates.yml b/changelogs/fragments/vm_updates.yml new file mode 100644 index 0000000000..ba0717ad3d --- /dev/null +++ b/changelogs/fragments/vm_updates.yml @@ -0,0 +1,2 @@ +minor_changes: + - moved some operations to inside VariableManager to make using it simpler and slightly optimized, but creating API changes diff --git a/lib/ansible/cli/__init__.py b/lib/ansible/cli/__init__.py index b09d9abf58..d4c496d72b 100644 --- a/lib/ansible/cli/__init__.py +++ b/lib/ansible/cli/__init__.py @@ -27,7 +27,6 @@ from ansible.parsing.dataloader import DataLoader from ansible.release import __version__ from ansible.utils.display import Display from ansible.utils.path import unfrackpath -from ansible.utils.vars import load_extra_vars, load_options_vars from ansible.vars.manager import VariableManager from ansible.parsing.vault import PromptVaultSecret, get_file_vault_secret @@ -520,19 +519,7 @@ class CLI(with_metaclass(ABCMeta, object)): # create the variable manager, which will be shared throughout # the code, ensuring a consistent view of global variables - variable_manager = VariableManager(loader=loader, inventory=inventory) - - # If the basedir is specified as the empty string then it results in cwd being used. This - # is not a safe location to load vars from - if options.get('basedir', False) is not False: - if basedir: - variable_manager.safe_basedir = True - else: - variable_manager.safe_basedir = True - - # load vars from cli options - variable_manager.extra_vars = load_extra_vars(loader=loader) - variable_manager.options_vars = load_options_vars(CLI.version_info(gitinfo=False)) + variable_manager = VariableManager(loader=loader, inventory=inventory, version_info=CLI.version_info(gitinfo=False)) return loader, inventory, variable_manager diff --git a/lib/ansible/utils/vars.py b/lib/ansible/utils/vars.py index deb2c161f6..bc6fece057 100644 --- a/lib/ansible/utils/vars.py +++ b/lib/ansible/utils/vars.py @@ -145,6 +145,8 @@ def load_extra_vars(loader): def load_options_vars(version): + if version is None: + version = 'Unknown' options_vars = {'ansible_version': version} attrs = {'check': 'check_mode', 'diff': 'diff_mode', diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py index 290d72b2d4..50346e43e8 100644 --- a/lib/ansible/vars/manager.py +++ b/lib/ansible/vars/manager.py @@ -43,7 +43,7 @@ from ansible.vars.fact_cache import FactCache from ansible.template import Templar from ansible.utils.display import Display from ansible.utils.listify import listify_lookup_plugin_terms -from ansible.utils.vars import combine_vars +from ansible.utils.vars import combine_vars, load_extra_vars, load_options_vars from ansible.utils.unsafe_proxy import wrap_var from ansible.vars.clean import namespace_facts, clean_facts @@ -76,7 +76,7 @@ class VariableManager: _ALLOWED = frozenset(['plugins_by_group', 'groups_plugins_play', 'groups_plugins_inventory', 'groups_inventory', 'all_plugins_play', 'all_plugins_inventory', 'all_inventory']) - def __init__(self, loader=None, inventory=None): + def __init__(self, loader=None, inventory=None, version_info=None): self._nonpersistent_fact_cache = defaultdict(dict) self._vars_cache = defaultdict(dict) self._extra_vars = defaultdict(dict) @@ -86,16 +86,25 @@ class VariableManager: self._loader = loader self._hostvars = None self._omit_token = '__omit_place_holder__%s' % sha1(os.urandom(64)).hexdigest() - self._options_vars = defaultdict(dict) - self.safe_basedir = False self._templar = Templar(loader=self._loader) - # bad cache plugin is not fatal error + self._options_vars = load_options_vars(version_info) + + # If the basedir is specified as the empty string then it results in cwd being used. + # This is not a safe location to load vars from. + basedir = self._options_vars.get('basedir', False) + self.safe_basedir = bool(basedir is False or basedir) + + # load extra vars + self._extra_vars = load_extra_vars(loader=self._loader) + + # load fact cache try: self._fact_cache = FactCache() except AnsibleError as e: - display.warning(to_native(e)) + # bad cache plugin is not fatal error # fallback to a dict as in memory cache + display.warning(to_native(e)) self._fact_cache = {} def __getstate__(self): @@ -127,31 +136,11 @@ class VariableManager: @property def extra_vars(self): - ''' ensures a clean copy of the extra_vars are made ''' - return self._extra_vars.copy() - - @extra_vars.setter - def extra_vars(self, value): - ''' ensures a clean copy of the extra_vars are used to set the value ''' - if not isinstance(value, Mapping): - raise AnsibleAssertionError("the type of 'value' for extra_vars should be a Mapping, but is a %s" % type(value)) - self._extra_vars = value.copy() + return self._extra_vars def set_inventory(self, inventory): self._inventory = inventory - @property - def options_vars(self): - ''' ensures a clean copy of the options_vars are made ''' - return self._options_vars.copy() - - @options_vars.setter - def options_vars(self, value): - ''' ensures a clean copy of the options_vars are used to set the value ''' - if not isinstance(value, dict): - raise AnsibleAssertionError("the type of 'value' for options_vars should be a dict, but is a %s" % type(value)) - self._options_vars = value.copy() - def get_vars(self, play=None, host=None, task=None, include_hostvars=True, include_delegate_to=True, use_cache=True): ''' Returns the variables, with optional "context" given via the parameters diff --git a/test/units/vars/test_variable_manager.py b/test/units/vars/test_variable_manager.py index d20e5047fe..f56ff42996 100644 --- a/test/units/vars/test_variable_manager.py +++ b/test/units/vars/test_variable_manager.py @@ -24,10 +24,9 @@ import os from collections import defaultdict from units.compat import unittest -from units.compat.mock import MagicMock, mock_open, patch +from units.compat.mock import MagicMock, patch from ansible.inventory.manager import InventoryManager from ansible.module_utils.six import iteritems -from ansible.module_utils.six.moves import builtins from ansible.playbook.play import Play @@ -63,14 +62,27 @@ class TestVariableManager(unittest.TestCase): extra_vars = dict(a=1, b=2, c=3) mock_inventory = MagicMock() v = VariableManager(loader=fake_loader, inventory=mock_inventory) - v.extra_vars = extra_vars - vars = v.get_vars(use_cache=False) + # override internal extra_vars loading + v._extra_vars = extra_vars + myvars = v.get_vars(use_cache=False) for (key, val) in iteritems(extra_vars): - self.assertEqual(vars.get(key), val) + self.assertEqual(myvars.get(key), val) - self.assertIsNot(v.extra_vars, extra_vars) + def test_variable_manager_options_vars(self): + fake_loader = DictDataLoader({}) + + options_vars = dict(a=1, b=2, c=3) + mock_inventory = MagicMock() + v = VariableManager(loader=fake_loader, inventory=mock_inventory) + + # override internal options_vars loading + v._extra_vars = options_vars + + myvars = v.get_vars(use_cache=False) + for (key, val) in iteritems(options_vars): + self.assertEqual(myvars.get(key), val) def test_variable_manager_play_vars(self): fake_loader = DictDataLoader({})