From 2f51105936b08da06056e2f5f6b7da528912eb80 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Mon, 8 Mar 2021 05:07:44 -0500 Subject: [PATCH] only add data when there is data to add (#54559) (#73566) Only add data when there is data to add also avoid clobbering existing data with empty file fixes #45843 * remove redundant code, update comments * fix mock dataloader, original does not return None * added test (cherry picked from commit ec8a5565386f514b34fb2b4969436672dfe5f71d) --- changelogs/fragments/fix_role_var_loading.yml | 2 + lib/ansible/playbook/helpers.py | 2 +- lib/ansible/playbook/role/__init__.py | 56 +++++++++++++------ .../targets/roles/data_integrity.yml | 4 ++ .../roles/roles/data/defaults/main/00.yml | 1 + .../roles/roles/data/defaults/main/01.yml | 0 .../targets/roles/roles/data/tasks/main.yml | 5 ++ test/integration/targets/roles/runme.sh | 4 ++ test/units/mock/loader.py | 5 +- 9 files changed, 60 insertions(+), 19 deletions(-) create mode 100644 changelogs/fragments/fix_role_var_loading.yml create mode 100644 test/integration/targets/roles/data_integrity.yml create mode 100644 test/integration/targets/roles/roles/data/defaults/main/00.yml create mode 100644 test/integration/targets/roles/roles/data/defaults/main/01.yml create mode 100644 test/integration/targets/roles/roles/data/tasks/main.yml diff --git a/changelogs/fragments/fix_role_var_loading.yml b/changelogs/fragments/fix_role_var_loading.yml new file mode 100644 index 00000000000..7328f6632b2 --- /dev/null +++ b/changelogs/fragments/fix_role_var_loading.yml @@ -0,0 +1,2 @@ +bugfixes: + - ensure we don't clobber role vars data when getting an empty file diff --git a/lib/ansible/playbook/helpers.py b/lib/ansible/playbook/helpers.py index 892ce158087..7f049018771 100644 --- a/lib/ansible/playbook/helpers.py +++ b/lib/ansible/playbook/helpers.py @@ -236,7 +236,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h try: data = loader.load_from_file(include_file) - if data is None: + if not data: display.warning('file %s is empty and had no tasks to include' % include_file) continue elif not isinstance(data, list): diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index b7456afcf77..30162c3ac6c 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -22,6 +22,7 @@ __metaclass__ = type import os from ansible.errors import AnsibleError, AnsibleParserError, AnsibleAssertionError +from ansible.module_utils._text import to_text from ansible.module_utils.six import iteritems, binary_type, text_type from ansible.module_utils.common._collections_compat import Container, Mapping, Set, Sequence from ansible.playbook.attribute import FieldAttribute @@ -33,8 +34,10 @@ from ansible.playbook.role.metadata import RoleMetadata from ansible.playbook.taggable import Taggable from ansible.plugins.loader import add_all_plugin_dirs from ansible.utils.collection_loader import AnsibleCollectionConfig +from ansible.utils.display import Display from ansible.utils.vars import combine_vars +display = Display() __all__ = ['Role', 'hash_params'] @@ -207,14 +210,14 @@ class Role(Base, Conditional, Taggable, CollectionSearch): # vars and default vars are regular dictionaries self._role_vars = self._load_role_yaml('vars', main=self._from_files.get('vars'), allow_dir=True) if self._role_vars is None: - self._role_vars = dict() - elif not isinstance(self._role_vars, dict): + self._role_vars = {} + elif not isinstance(self._role_vars, Mapping): raise AnsibleParserError("The vars/main.yml file for role '%s' must contain a dictionary of variables" % self._role_name) self._default_vars = self._load_role_yaml('defaults', main=self._from_files.get('defaults'), allow_dir=True) if self._default_vars is None: - self._default_vars = dict() - elif not isinstance(self._default_vars, dict): + self._default_vars = {} + elif not isinstance(self._default_vars, Mapping): raise AnsibleParserError("The defaults/main.yml file for role '%s' must contain a dictionary of variables" % self._role_name) # load the role's other files, if they exist @@ -269,32 +272,53 @@ class Role(Base, Conditional, Taggable, CollectionSearch): obj=handler_data, orig_exc=e) def _load_role_yaml(self, subdir, main=None, allow_dir=False): + ''' + Find and load role YAML files and return data found. + :param subdir: subdir of role to search (vars, files, tasks, handlers, defaults) + :type subdir: string + :param main: filename to match, will default to 'main.' if not provided. + :type main: string + :param allow_dir: If true we combine results of multiple matching files found. + If false, highlander rules. Only for vars(dicts) and not tasks(lists). + :type allow_dir: bool + + :returns: data from the matched file(s), type can be dict or list depending on vars or tasks. + ''' + data = None file_path = os.path.join(self._role_path, subdir) if self._loader.path_exists(file_path) and self._loader.is_directory(file_path): - # Valid extensions and ordering for roles is hard-coded to maintain - # role portability - extensions = ['.yml', '.yaml', '.json'] - # If no
is specified by the user, look for files with - # extensions before bare name. Otherwise, look for bare name first. + # Valid extensions and ordering for roles is hard-coded to maintain portability + extensions = ['.yml', '.yaml', '.json'] # same as default for YAML_FILENAME_EXTENSIONS + + # look for files w/o extensions before/after bare name depending on it being set or not + # keep 'main' as original to figure out errors if no files found if main is None: _main = 'main' extensions.append('') else: _main = main extensions.insert(0, '') + + # not really 'find_vars_files' but find_files_with_extensions_default_to_yaml_filename_extensions found_files = self._loader.find_vars_files(file_path, _main, extensions, allow_dir) if found_files: - data = {} for found in found_files: new_data = self._loader.load_from_file(found) - if new_data and allow_dir: - data = combine_vars(data, new_data) - else: - data = new_data - return data + if new_data: + if data is not None and isinstance(new_data, Mapping): + data = combine_vars(data, new_data) + else: + data = new_data + + # found data so no need to continue unless we want to merge + if not allow_dir: + break + elif main is not None: + # this won't trigger with default only when _from is specified raise AnsibleParserError("Could not find specified file in role: %s/%s" % (subdir, main)) - return None + + return data def _load_dependencies(self): ''' diff --git a/test/integration/targets/roles/data_integrity.yml b/test/integration/targets/roles/data_integrity.yml new file mode 100644 index 00000000000..5eb4fb32bc8 --- /dev/null +++ b/test/integration/targets/roles/data_integrity.yml @@ -0,0 +1,4 @@ +- hosts: all + gather_facts: false + roles: + - data diff --git a/test/integration/targets/roles/roles/data/defaults/main/00.yml b/test/integration/targets/roles/roles/data/defaults/main/00.yml new file mode 100644 index 00000000000..98c13a15856 --- /dev/null +++ b/test/integration/targets/roles/roles/data/defaults/main/00.yml @@ -0,0 +1 @@ +defined_var: 1 diff --git a/test/integration/targets/roles/roles/data/defaults/main/01.yml b/test/integration/targets/roles/roles/data/defaults/main/01.yml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/integration/targets/roles/roles/data/tasks/main.yml b/test/integration/targets/roles/roles/data/tasks/main.yml new file mode 100644 index 00000000000..8d85580c825 --- /dev/null +++ b/test/integration/targets/roles/roles/data/tasks/main.yml @@ -0,0 +1,5 @@ +- name: ensure data was correctly defind + assert: + that: + - defined_var is defined + - defined_var == 1 diff --git a/test/integration/targets/roles/runme.sh b/test/integration/targets/roles/runme.sh index fe99ea10b63..f2058ff157f 100755 --- a/test/integration/targets/roles/runme.sh +++ b/test/integration/targets/roles/runme.sh @@ -12,3 +12,7 @@ set -eux # include/import can execute another instance of role [ "$(ansible-playbook allowed_dupes.yml -i ../../inventory --tags importrole "$@" | grep -c '"msg": "A"')" = "2" ] [ "$(ansible-playbook allowed_dupes.yml -i ../../inventory --tags includerole "$@" | grep -c '"msg": "A"')" = "2" ] + + +# ensure role data is merged correctly +ansible-playbook data_integrity.yml -i ../../inventory "$@" diff --git a/test/units/mock/loader.py b/test/units/mock/loader.py index 0ee47fbb9f5..c47ec39e8c4 100644 --- a/test/units/mock/loader.py +++ b/test/units/mock/loader.py @@ -39,10 +39,11 @@ class DictDataLoader(DataLoader): self._vault_secrets = None def load_from_file(self, path, cache=True, unsafe=False): + data = None path = to_text(path) if path in self._file_mapping: - return self.load(self._file_mapping[path], path) - return None + data = self.load(self._file_mapping[path], path) + return data # TODO: the real _get_file_contents returns a bytestring, so we actually convert the # unicode/text it's created with to utf-8