[Pkg-puppet-devel] [facter] 213/352: (maint) Don't warn on non-existent paths

[Stig Sandbeck Mathisen]

This is an automated email from the git hooks/post-receive script.

ssm pushed a commit to branch master
in repository facter.

commit 319abe8b9a61061ca3d75edf762db7d887317d5e
Author: Adrien Thebo <git at somethingsinistral.net>
Date:   Wed Feb 5 09:43:11 2014 -0800

    (maint) Don't warn on non-existent paths
    
    Facter was emitting warnings whenever a path specified by
    `Facter.search_path` was not a directory, but Puppet was automatically
    adding two paths via the `factpath` setting that seem to never exist.
    This could cause hundreds of warnings to be generated, none of which are
    really that useful. This commit changes the warnings to only be emitted
    when a manually specified path is not absolute and silently ignores all
    paths that aren't directories.
---
 lib/facter/util/loader.rb     | 43 ++++++++++++++++--------------------
 spec/unit/util/loader_spec.rb | 51 ++++++++++++++++++++++++++-----------------
 2 files changed, 50 insertions(+), 44 deletions(-)

diff --git a/lib/facter/util/loader.rb b/lib/facter/util/loader.rb
index a8fb88d..25339fe 100644
--- a/lib/facter/util/loader.rb
+++ b/lib/facter/util/loader.rb
@@ -54,56 +54,51 @@ class Facter::Util::Loader
     @loaded_all = true
   end
 
-  # List of directories to search for fact files.
+  # List directories to search for fact files.
   #
   # Search paths are gathered from the following sources:
   #
   # 1. $LOAD_PATH entries are expanded to absolute paths
   # 2. ENV['FACTERLIB'] is split and used verbatim
-  # 3. Entries from Facter::search_path are used verbatim
+  # 3. Entries from Facter.search_path are used verbatim
   #
-  # A warning will be generated for any path(s) from Facter::search_path that
-  # are not an absolute path to an existing directory.
+  # A warning will be generated for paths in Facter.search_path that are not
+  # absolute directories.
   #
   # @api public
   # @return [Array<String>]
   def search_path
-    result = []
-    result += $LOAD_PATH.map { |path| File.expand_path('facter', path) }
+    search_paths = []
+    search_paths += $LOAD_PATH.map { |path| File.expand_path('facter', path) }
 
     if @environment_vars.include?("FACTERLIB")
-      result += @environment_vars["FACTERLIB"].split(File::PATH_SEPARATOR)
+      search_paths += @environment_vars["FACTERLIB"].split(File::PATH_SEPARATOR)
     end
 
-    # silently ignore bad search paths from $LOAD_PATH and FACTERLIB
-    result = result.select { |path| valid_search_path?(path) }
+    search_paths.select! { |path| valid_search_path?(path) }
 
-    # This allows others to register additional paths we should search.
-    # We are assuming that these are already absolute paths.
-    result += Facter.search_path.select do |path|
-      valid = valid_search_path?(path)
-      Facter.warn "Excluding #{path} from search path. Fact file paths must be an absolute directory" unless valid
-      valid
+    Facter.search_path.each do |path|
+      if valid_search_path?(path)
+        search_paths << path
+      else
+        Facter.warn "Excluding #{path} from search path. Fact file paths must be an absolute directory"
+      end
     end
 
-    # remove any dups
-    result.uniq
+    search_paths.select! { |path| File.directory?(path) }
+
+    search_paths.uniq
   end
 
   private
 
-  # Validate that a path string is a valid to use for loading loading fact .rb
-  # files from.  The path must both be absolute and a directory.
+  # Validate that the given path is valid, ie it is an absolute path.
   #
   # @api private
   # @param path [String]
   # @return [Boolean]
   def valid_search_path?(path)
-    unless File.directory?(path) and Pathname.new(path).absolute?
-      return false
-    end
-
-    true
+    Pathname.new(path).absolute?
   end
 
   # Load a file and record is paths to prevent duplicate loads.
diff --git a/spec/unit/util/loader_spec.rb b/spec/unit/util/loader_spec.rb
index 8ba1185..2867f36 100755
--- a/spec/unit/util/loader_spec.rb
+++ b/spec/unit/util/loader_spec.rb
@@ -51,15 +51,7 @@ describe Facter::Util::Loader do
       ' /',
       ' \/',
     ].each do |dir|
-      it "should be false for relative path to non-directory #{dir}" do
-        File.stubs(:directory?).with(dir).returns false
-
-        @loader.should_not be_valid_search_path dir
-      end
-
-      it "should be false for relative path to directory #{dir}" do
-        File.stubs(:directory?).with(dir).returns true
-
+      it "should be false for relative path #{dir}" do
         @loader.should_not be_valid_search_path dir
       end
     end
@@ -75,15 +67,7 @@ describe Facter::Util::Loader do
       '/ ',
       '/ /..',
     ].each do |dir|
-      it "should be false for absolute path to non-directory #{dir}" do
-        File.stubs(:directory?).with(dir).returns false
-
-        @loader.should_not be_valid_search_path dir
-      end
-
-      it "should be true for absolute path to directory #{dir}" do
-        File.stubs(:directory?).with(dir).returns true
-
+      it "should be true for absolute path #{dir}" do
         @loader.should be_valid_search_path dir
       end
     end
@@ -119,26 +103,53 @@ describe Facter::Util::Loader do
       Facter.expects(:search_path).returns %w{/one /two}
       @loader.stubs(:valid_search_path?).returns true
 
+      File.stubs(:directory?).returns false
+      File.stubs(:directory?).with('/one').returns true
+      File.stubs(:directory?).with('/two').returns true
+
       paths = @loader.search_path
       paths.should be_include("/one")
       paths.should be_include("/two")
     end
 
     it "should warn on invalid search paths registered with Facter" do
+      Facter.expects(:search_path).returns %w{/one two/three}
+      @loader.stubs(:valid_search_path?).returns false
+      @loader.stubs(:valid_search_path?).with('/one').returns true
+      @loader.stubs(:valid_search_path?).with('two/three').returns false
+      Facter.expects(:warn).with('Excluding two/three from search path. Fact file paths must be an absolute directory').once
+
+      File.stubs(:directory?).returns false
+      File.stubs(:directory?).with('/one').returns true
+
+      paths = @loader.search_path
+      paths.should be_include("/one")
+      paths.should_not be_include("two/three")
+    end
+
+    it "should strip paths that are valid paths but not are not present" do
       Facter.expects(:search_path).returns %w{/one /two}
       @loader.stubs(:valid_search_path?).returns false
       @loader.stubs(:valid_search_path?).with('/one').returns true
-      @loader.stubs(:valid_search_path?).with('/two').returns false
-      Facter.expects(:warn).with('Excluding /two from search path. Fact file paths must be an absolute directory').once
+      @loader.stubs(:valid_search_path?).with('/two').returns true
+
+      File.stubs(:directory?).returns false
+      File.stubs(:directory?).with('/one').returns true
+      File.stubs(:directory?).with('/two').returns false
 
       paths = @loader.search_path
       paths.should be_include("/one")
+      paths.should_not be_include('/two')
     end
 
     describe "and the FACTERLIB environment variable is set" do
       it "should include all paths in FACTERLIB" do
         loader = Facter::Util::Loader.new("FACTERLIB" => "/one/path#{File::PATH_SEPARATOR}/two/path")
 
+      File.stubs(:directory?).returns false
+      File.stubs(:directory?).with('/one/path').returns true
+      File.stubs(:directory?).with('/two/path').returns true
+
         loader.stubs(:valid_search_path?).returns true
         paths = loader.search_path
         %w{/one/path /two/path}.each do |dir|

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-puppet/facter.git