From 8f758204cf379a743332bc9865fb3c002b6c3a6a Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Mon, 3 Jul 2017 15:27:53 -0400 Subject: [PATCH] correct, cleanup & simplify dwim stack (#25956) * correct, cleanup & simplify dwim stack latlh chIS logh HeS qar wej chel laD better errors update find_file to new exception * addressed latest comments * test should not use realpath as it follows symlink this fails when on OS X as /var is now a symlink to /private/var but first_found was not supposed to follow symlinks --- lib/ansible/errors/__init__.py | 22 ++++- lib/ansible/parsing/dataloader.py | 81 ++++++++++--------- lib/ansible/plugins/action/__init__.py | 8 +- lib/ansible/plugins/lookup/__init__.py | 10 ++- lib/ansible/vars/manager.py | 6 +- .../targets/iterators/tasks/main.yml | 4 +- .../targets/win_copy/tasks/main.yml | 2 +- 7 files changed, 81 insertions(+), 52 deletions(-) diff --git a/lib/ansible/errors/__init__.py b/lib/ansible/errors/__init__.py index 97ee3fc413f..c3249d9679c 100644 --- a/lib/ansible/errors/__init__.py +++ b/lib/ansible/errors/__init__.py @@ -19,6 +19,8 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +from collections import Sequence + from ansible.errors.yaml_strings import ( YAML_COMMON_DICT_ERROR, YAML_COMMON_LEADING_TAB_ERROR, @@ -220,7 +222,25 @@ class AnsibleUndefinedVariable(AnsibleRuntimeError): class AnsibleFileNotFound(AnsibleRuntimeError): ''' a file missing failure ''' - pass + + def __init__(self, message="", obj=None, show_content=True, suppress_extended_error=False, orig_exc=None, paths=None, file_name=None): + + self.file_name = file_name + self.paths = paths + + if self.file_name: + if message: + message += "\n" + message += "Could not find or access '%s'" % to_text(self.file_name) + + if self.paths and isinstance(self.paths, Sequence): + searched = to_text('\n\t'.join(self.paths)) + if message: + message += "\n" + message += "Searched in:\n\t%s" % searched + + super(AnsibleFileNotFound, self).__init__(message=message, obj=obj, show_content=show_content, + suppress_extended_error=suppress_extended_error, orig_exc=orig_exc) class AnsibleActionSkip(AnsibleRuntimeError): diff --git a/lib/ansible/parsing/dataloader.py b/lib/ansible/parsing/dataloader.py index a2579254701..ac502d5eeff 100644 --- a/lib/ansible/parsing/dataloader.py +++ b/lib/ansible/parsing/dataloader.py @@ -22,6 +22,7 @@ __metaclass__ = type import copy import os import json +import re import tempfile from yaml import YAMLError @@ -42,6 +43,10 @@ except ImportError: from ansible.utils.display import Display display = Display() +# Tries to determine if a path is inside a role, last dir must be 'tasks' +# this is not perfect but people should really avoid 'tasks' dirs outside roles when using Ansible. +RE_TASKS = re.compile(u'(?:^|%s)+tasks%s?$' % (os.path.sep, os.path.sep)) + class DataLoader: @@ -184,7 +189,7 @@ class DataLoader: b_file_name = to_bytes(file_name) if not self.path_exists(b_file_name) or not self.is_file(b_file_name): - raise AnsibleFileNotFound("the file named '%s' does not exist, or is not readable" % file_name) + raise AnsibleFileNotFound("Unable to retrieve file contents", file_name=file_name) show_content = True try: @@ -233,35 +238,33 @@ class DataLoader: given = unquote(given) given = to_text(given, errors='surrogate_or_strict') - if given.startswith(u"/"): - return os.path.abspath(given) - elif given.startswith(u"~"): - return os.path.abspath(os.path.expanduser(given)) + if given.startswith(to_text(os.path.sep)) or given.startswith(u'~'): + path = given else: basedir = to_text(self._basedir, errors='surrogate_or_strict') - return os.path.abspath(os.path.join(basedir, given)) + path = os.path.join(basedir, given) + + return unfrackpath(path, follow=False) def _is_role(self, path): - ''' imperfect role detection, roles are still valid w/o main.yml/yaml/etc ''' + ''' imperfect role detection, roles are still valid w/o tasks|meta/main.yml|yaml|etc ''' - isit = False b_path = to_bytes(path, errors='surrogate_or_strict') - b_upath = to_bytes(unfrackpath(path), errors='surrogate_or_strict') + b_upath = to_bytes(unfrackpath(path, follow=False), errors='surrogate_or_strict') - for suffix in (b'.yml', b'.yaml', b''): - b_main = b'main%s' % (suffix) - b_tasked = b'tasks/%s' % (b_main) + for finddir in (b'meta', b'tasks'): + for suffix in (b'.yml', b'.yaml', b''): + b_main = b'main%s' % (suffix) + b_tasked = b'%s/%s' % (finddir, b_main) - if ( - b_path.endswith(b'tasks') and - os.path.exists(os.path.join(b_path, b_main)) or - os.path.exists(os.path.join(b_upath, b_tasked)) or - os.path.exists(os.path.join(os.path.dirname(b_path), b_tasked)) - ): - isit = True - break - - return isit + if ( + RE_TASKS.search(path) and + os.path.exists(os.path.join(b_path, b_main)) or + os.path.exists(os.path.join(b_upath, b_tasked)) or + os.path.exists(os.path.join(os.path.dirname(b_path), b_tasked)) + ): + return True + return False def path_dwim_relative(self, path, dirname, source, is_role=False): ''' @@ -273,35 +276,35 @@ class DataLoader: ''' search = [] + source = to_text(source, errors='surrogate_or_strict') # I have full path, nothing else needs to be looked at - if source.startswith('~') or source.startswith(os.path.sep): - search.append(self.path_dwim(source)) + if source.startswith(to_text(os.path.sep)) or source.startswith(u'~'): + search.append(unfrackpath(source, follow=False)) else: # base role/play path + templates/files/vars + relative filename search.append(os.path.join(path, dirname, source)) - basedir = unfrackpath(path) + basedir = unfrackpath(path, follow=False) # not told if role, but detect if it is a role and if so make sure you get correct base path if not is_role: is_role = self._is_role(path) - if is_role and path.endswith('tasks'): - basedir = unfrackpath(os.path.dirname(path)) + if is_role and RE_TASKS.search(path): + basedir = unfrackpath(os.path.dirname(path), follow=False) cur_basedir = self._basedir self.set_basedir(basedir) # resolved base role/play path + templates/files/vars + relative filename - search.append(self.path_dwim(os.path.join(basedir, dirname, source))) + search.append(unfrackpath(os.path.join(basedir, dirname, source), follow=False)) self.set_basedir(cur_basedir) if is_role and not source.endswith(dirname): # look in role's tasks dir w/o dirname - search.append(self.path_dwim(os.path.join(basedir, 'tasks', source))) + search.append(unfrackpath(os.path.join(basedir, 'tasks', source), follow=False)) # try to create absolute path for loader basedir + templates/files/vars + filename - search.append(self.path_dwim(os.path.join(dirname, source))) - search.append(self.path_dwim(os.path.join(basedir, source))) + search.append(unfrackpath(os.path.join(dirname, source), follow=False)) # try to create absolute path for loader basedir + filename search.append(self.path_dwim(source)) @@ -321,24 +324,25 @@ class DataLoader: is prepended to the source to form the path to search for. :arg source: A text string which is the filename to search for :rtype: A text string - :returns: An absolute path to the filename ``source`` + :returns: An absolute path to the filename ``source`` if found + :raises: An AnsibleFileNotFound Exception if the file is found to exist in the search paths ''' b_dirname = to_bytes(dirname) b_source = to_bytes(source) result = None + search = [] if source is None: display.warning('Invalid request to find a file that matches a "null" value') elif source and (source.startswith('~') or source.startswith(os.path.sep)): # path is absolute, no relative needed, check existence and return source - test_path = unfrackpath(b_source) + test_path = unfrackpath(b_source, follow=False) if os.path.exists(to_bytes(test_path, errors='surrogate_or_strict')): result = test_path else: - search = [] display.debug(u'evaluation_path:\n\t%s' % '\n\t'.join(paths)) for path in paths: - upath = unfrackpath(path) + upath = unfrackpath(path, follow=False) b_upath = to_bytes(upath, errors='surrogate_or_strict') b_mydir = os.path.dirname(b_upath) @@ -373,6 +377,9 @@ class DataLoader: result = to_text(b_candidate) break + if result is None: + raise AnsibleFileNotFound(file_name=source, paths=search) + return result def _create_content_tempfile(self, content): @@ -401,7 +408,7 @@ class DataLoader: b_file_path = to_bytes(file_path, errors='surrogate_or_strict') if not self.path_exists(b_file_path) or not self.is_file(b_file_path): - raise AnsibleFileNotFound("the file named '%s' does not exist, or is not accessible" % to_native(file_path)) + raise AnsibleFileNotFound(file_name=file_path) if not self._vault: self._vault = VaultLib(b_password="") @@ -420,7 +427,7 @@ class DataLoader: # since the decrypt function doesn't know the file name data = f.read() if not self._b_vault_password: - raise AnsibleParserError("A vault password must be specified to decrypt %s" % file_path) + raise AnsibleParserError("A vault password must be specified to decrypt %s" % to_native(file_path)) data = self._vault.decrypt(data, filename=real_path) # Make a temp file diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index c2aff519deb..f222f8e11b3 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -981,9 +981,5 @@ class ActionBase(with_metaclass(ABCMeta, object)): # dwim already deals with playbook basedirs path_stack = self._task.get_search_path() - result = self._loader.path_dwim_relative_stack(path_stack, dirname, needle) - - if result is None: - raise AnsibleError("Unable to find '%s' in expected paths." % to_native(needle)) - - return result + # if missing it will return a file not found exception + return self._loader.path_dwim_relative_stack(path_stack, dirname, needle) diff --git a/lib/ansible/plugins/lookup/__init__.py b/lib/ansible/plugins/lookup/__init__.py index eaecd62252d..0b2036aaa57 100644 --- a/lib/ansible/plugins/lookup/__init__.py +++ b/lib/ansible/plugins/lookup/__init__.py @@ -22,6 +22,7 @@ __metaclass__ = type from abc import ABCMeta, abstractmethod from ansible.module_utils.six import with_metaclass +from ansible.errors import AnsibleFileNotFound try: from __main__ import display @@ -112,8 +113,11 @@ class LookupBase(with_metaclass(ABCMeta, object)): else: paths = self.get_basedir(myvars) - result = self._loader.path_dwim_relative_stack(paths, subdir, needle) - if result is None and not ignore_missing: - self._display.warning("Unable to find '%s' in expected paths." % needle) + result = None + try: + result = self._loader.path_dwim_relative_stack(paths, subdir, needle) + except AnsibleFileNotFound: + if not ignore_missing: + self._display.warning("Unable to find '%s' in expected paths." % needle) return result diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py index 6b232bce2a1..c3a64d680df 100644 --- a/lib/ansible/vars/manager.py +++ b/lib/ansible/vars/manager.py @@ -369,11 +369,13 @@ class VariableManager: raise AnsibleUndefinedVariable("an undefined variable was found when attempting to template the vars_files item '%s'" % vars_file_item, obj=vars_file_item) else: - # we do not have a full context here, and the missing variable could be - # because of that, so just show a warning and continue + # we do not have a full context here, and the missing variable could be because of that + # so just show a warning and continue display.vvv("skipping vars_file '%s' due to an undefined variable" % vars_file_item) continue + display.vvv("Read vars_file '%s'" % vars_file_item) + # By default, we now merge in all vars from all roles in the play, # unless the user has disabled this via a config option if not C.DEFAULT_PRIVATE_ROLE_VARS: diff --git a/test/integration/targets/iterators/tasks/main.yml b/test/integration/targets/iterators/tasks/main.yml index bc66dde318d..a76f40242e4 100644 --- a/test/integration/targets/iterators/tasks/main.yml +++ b/test/integration/targets/iterators/tasks/main.yml @@ -216,10 +216,10 @@ - "{{ output_dir + '/bar1' }}" - name: set expected - set_fact: first_expected="{{ output_dir | expanduser | realpath + '/foo1' }}" + set_fact: first_expected="{{ output_dir | expanduser + '/foo1' }}" - name: set unexpected - set_fact: first_unexpected="{{ output_dir | expanduser | realpath + '/bar1' }}" + set_fact: first_unexpected="{{ output_dir | expanduser + '/bar1' }}" - name: verify with_first_found results assert: diff --git a/test/integration/targets/win_copy/tasks/main.yml b/test/integration/targets/win_copy/tasks/main.yml index 3e7e2b2a593..bea2be8660c 100644 --- a/test/integration/targets/win_copy/tasks/main.yml +++ b/test/integration/targets/win_copy/tasks/main.yml @@ -329,7 +329,7 @@ src: false-file dest: "{{win_output_dir}}\\fale-file.txt" register: fail_missing_source - failed_when: fail_missing_source.msg != "Unable to find 'false-file' in expected paths." + failed_when: not (fail_missing_source|failed) - name: fail when copying remote src file when src doesn't exist win_copy: