refactor to catch edge cases, remove repeated code

- Move all the supported YAML file extensions into a constant
- Use helper functions to avoid duplicate code for group/host vars
- Catch and disallow some confusing situations, such as the presence of
  multiple group/host vars files for the same group/host, but with
  different extensions.  For example having both group_vars/all.yml and
  group_vars/all.yaml.
- Catch and report file system permission issues, symlink errors,
  unexpected file system objects
- Trivial performance improvement from making fewer stat system calls
- Restructuring that makes it easy for a following patch to support
  directory recursion

lib/ansible/constants.py

@@ -89,6 +89,10 @@ if getattr(sys, "real_prefix", None):
 else:
     DIST_MODULE_PATH = '/usr/share/ansible/'
 
+# check all of these extensions when looking for yaml files for things like
+# group variables
+YAML_FILENAME_EXTENSIONS = [ "", ".yml", ".yaml" ]
+
 # sections in config file
 DEFAULTS='defaults'
 

lib/ansible/inventory/vars_plugins/group_vars.py

@@ -16,11 +16,99 @@
 # along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import glob
+import stat
+import errno
+
 from ansible import errors
 from ansible import utils
 import ansible.constants as C
 
+def _load_vars(basepath, results):
+    """
+    Load variables from any potential yaml filename combinations of basepath,
+    returning result.
+    """
+
+    paths_to_check = [ "".join([basepath, ext]) 
+                       for ext in C.YAML_FILENAME_EXTENSIONS ]
+
+    found_paths = []
+
+    for path in paths_to_check:
+        found, results = _load_vars_from_path(path, results)
+        if found:
+            found_paths.append(path)
+
+
+    # disallow the potentially confusing situation that there are multiple
+    # variable files for the same name. For example if both group_vars/all.yml
+    # and group_vars/all.yaml
+    if len(found_paths) > 1:
+        raise errors.AnsibleError("Multiple variable files found. "
+            "There should only be one. %s" % ( found_paths, ))
+
+    return results
+
+def _load_vars_from_path(path, results):
+    """
+    Robustly access the file at path and load variables, carefully reporting
+    errors in a friendly/informative way.
+
+    Return the tuple (found, new_results, )
+    """
+
+    try:
+        # in the case of a symbolic link, we want the stat of the link itself,
+        # not its target
+        pathstat = os.lstat(path)
+    except os.error, err:
+        # most common case is that nothing exists at that path.
+        if err.errno == errno.ENOENT:
+            return False, results
+        # otherwise this is a condition we should report to the user
+        raise errors.AnsibleError(
+            "%s is not accessible: %s." 
+            " Please check its permissions." % ( path, err.strerror))
+
+    # symbolic link
+    if stat.S_ISLNK(pathstat.st_mode):
+        try:
+            target = os.readlink(path)
+        except os.error, err2:
+            raise errors.AnsibleError("The symbolic link at %s "
+                "is not readable: %s.  Please check its permissions."
+                % (path, err2.strerror, ))
+        # follow symbolic link chains by recursing, so we repeat the same
+        # permissions checks above and provide useful errors.
+        return _load_vars_from_path(target, results)
+
+    # directory
+    if stat.S_ISDIR(pathstat.st_mode):
+
+        # we ignore directories. however in the future it may be useful to
+        # allow variables to be contained in multiple files in the named
+        # directory, so that users can better organize large variables
+        # across multiple files.
+        raise errors.AnsibleError("Expected a variable file at path %s "
+            "but found a directory instead." % (path, ))
+
+    # regular file
+    elif stat.S_ISREG(pathstat.st_mode):
+        data = utils.parse_yaml_from_file(path)
+        if type(data) != dict:
+            raise errors.AnsibleError(
+                "%s must be stored as a dictionary/hash" % path)
+
+        # combine vars overrides by default but can be configured to do a
+        # hash merge in settings
+        results = utils.combine_vars(results, data)
+        return True, results
+
+    # something else? could be a fifo, socket, device, etc.
+    else:
+        raise errors.AnsibleError("Expected a variable file but found a "
+            "non-file object at path %s" % (path, ))
+
 class VarsModule(object):
 
     """
@@ -73,42 +161,13 @@ class VarsModule(object):
                 continue
 
             # load vars in dir/group_vars/name_of_group
-            for x in groups:
+            for group in groups:
+                base_path = os.path.join(basedir, "group_vars/%s" % group)
+                results = _load_vars(base_path, results)
 
-                p = os.path.join(basedir, "group_vars/%s" % x)
-
-                # the file can be <groupname> or end in .yml or .yaml
-                # currently ALL will be loaded, even if more than one
-                paths = [p, '.'.join([p, 'yml']), '.'.join([p, 'yaml'])]
-
-                for path in paths:
-
-                    if os.path.exists(path) and not os.path.isdir(path):
-                        data = utils.parse_yaml_from_file(path)
-                        if type(data) != dict:
-                            raise errors.AnsibleError("%s must be stored as a dictionary/hash" % path)
-
-                        # combine vars overrides by default but can be configured to do a hash
-                        # merge in settings
-
-                        results = utils.combine_vars(results, data)
-
-            # group vars have been loaded
-            # load vars in inventory_dir/hosts_vars/name_of_host
-            # these have greater precedence than group variables
-
-            p = os.path.join(basedir, "host_vars/%s" % host.name)
-
-            # again allow the file to be named filename or end in .yml or .yaml
-            paths = [p, '.'.join([p, 'yml']), '.'.join([p, 'yaml'])]
-
-            for path in paths:
-
-                if os.path.exists(path) and not os.path.isdir(path):
-                    data = utils.parse_yaml_from_file(path)
-                    if type(data) != dict:
-                        raise errors.AnsibleError("%s must be stored as a dictionary/hash" % path)
-                    results = utils.combine_vars(results, data)
+            # same for hostvars in dir/host_vars/name_of_host
+            base_path = os.path.join(basedir, "host_vars/%s" % host.name)
+            results = _load_vars(base_path, results)
 
         # all done, results is a dictionary of variables for this particular host.
         return results