[Pkg-puppet-devel] [facter] 77/352: (FACT-163) refactor fact [file] loading
Stig Sandbeck Mathisen
ssm at debian.org
Sun Apr 6 22:21:33 UTC 2014
This is an automated email from the git hooks/post-receive script.
ssm pushed a commit to branch master
in repository facter.
commit 4e41e421afb080e7409c4bca280c7f9a09ecce01
Author: Joshua Hoblitt <jhoblitt at cpan.org>
Date: Mon Dec 16 21:52:10 2013 -0800
(FACT-163) refactor fact [file] loading
This patch simplifies the logic for locating and loading fact .rb files. It
reduces the search space for a fact files to only the configured search dir
paths (non-recursively).
---
lib/facter/util/loader.rb | 103 +++++++++++++--------
spec/unit/util/loader_spec.rb | 206 ++++++++++++++----------------------------
2 files changed, 132 insertions(+), 177 deletions(-)
diff --git a/lib/facter/util/loader.rb b/lib/facter/util/loader.rb
index f0e5fee..6001f15 100644
--- a/lib/facter/util/loader.rb
+++ b/lib/facter/util/loader.rb
@@ -7,43 +7,46 @@ class Facter::Util::Loader
def initialize
@loaded = []
- @valid_path = {}
end
# Load all resolutions for a single fact.
+ #
+ # @api public
+ # @param name [Symbol]
def load(fact)
# Now load from the search path
shortname = fact.to_s.downcase
load_env(shortname)
filename = shortname + ".rb"
- search_path.each do |dir|
- # Load individual files
- file = File.join(dir, filename)
- load_file(file) if FileTest.exist?(file)
+ paths = search_path
+ unless paths.nil?
+ paths.each do |dir|
+ # Load individual files
+ file = File.join(dir, filename)
- # And load any directories matching the name
- factdir = File.join(dir, shortname)
- load_dir(factdir) if FileTest.directory?(factdir)
+ load_file(file) if File.file?(file)
+ end
end
end
# Load all facts from all directories.
+ #
+ # @api public
def load_all
return if defined?(@loaded_all)
load_env
- search_path.each do |dir|
- next unless FileTest.directory?(dir)
-
- Dir.entries(dir).sort.each do |file|
- path = File.join(dir, file)
- if File.directory?(path)
- load_dir(path)
- elsif file =~ /\.rb$/
- load_file(File.join(dir, file))
+ paths = search_path
+ unless paths.nil?
+ paths.each do |dir|
+ # dir is already an absolute path
+ Dir.glob(File.join(dir, '*.rb')).each do |dirent|
+ path = File.join(dir, dirent)
+ # exclude dirs that end with .rb
+ load_file(path) if File.file?(path)
end
end
end
@@ -51,43 +54,67 @@ class Facter::Util::Loader
@loaded_all = true
end
- # The list of directories we're going to search through for facts.
+ # List of 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 expanded to absolute paths
+ # 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.
+ #
+ # @api public
+ # @return [Array<String>]
def search_path
result = []
- result += $LOAD_PATH.collect { |d| File.join(d, "facter") }
- if ENV.include?("FACTERLIB")
- result += ENV["FACTERLIB"].split(File::PATH_SEPARATOR)
+ result += $LOAD_PATH.map { |path| File.expand_path('facter', path) }
+
+ if ENV.include?('FACTERLIB')
+ ENV['FACTERLIB'].split(File::PATH_SEPARATOR).each do |path|
+ result << File.expand_path(path)
+ end
end
- # This allows others to register additional paths we should search.
- result += Facter.search_path
+ # silently ignore bad search paths from $LOAD_PATH and FACTERLIB
+ result = result.select { |path| valid_search_path?(path) }
- result.select do |dir|
- good = valid_search_path? dir
- Facter.debugonce("Relative directory #{dir} removed from search path.") unless good
- good
+ # 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
end
- end
- def valid_search_path?(path)
- return @valid_path[path] unless @valid_path[path].nil?
-
- return @valid_path[path] = Pathname.new(path).absolute?
+ # remove any dups
+ result.uniq
end
- private :valid_search_path?
private
- def load_dir(dir)
- return if dir =~ /\/\.+$/ or dir =~ /\/util$/ or dir =~ /\/lib$/
-
- Dir.entries(dir).find_all { |f| f =~ /\.rb$/ }.sort.each do |file|
- load_file(File.join(dir, file))
+ # 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.
+ #
+ # @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
end
+ # Load a file and record is paths to prevent duplicate loads.
+ #
+ # @api private
+ # @params file [String] The *absolute path* to the file to load
def load_file(file)
return if @loaded.include? file
+
# We have to specify Kernel.load, because we have a load method.
begin
# Store the file path so we don't try to reload it
diff --git a/spec/unit/util/loader_spec.rb b/spec/unit/util/loader_spec.rb
index d0b8706..632db37 100755
--- a/spec/unit/util/loader_spec.rb
+++ b/spec/unit/util/loader_spec.rb
@@ -3,19 +3,6 @@
require 'spec_helper'
require 'facter/util/loader'
-# loader subclass for making assertions about file/directory ordering
-class TestLoader < Facter::Util::Loader
- def loaded_files
- @loaded_files ||= []
- end
-
- def load_file(file)
- loaded_files << file
- super
- end
-end
-
-
describe Facter::Util::Loader do
before :each do
Facter::Util::Loader.any_instance.unstub(:load_all)
@@ -40,15 +27,6 @@ describe Facter::Util::Loader do
@settings.stubs(:value).returns "/eh"
end
- it "should cache the result of a previous check" do
- Pathname.any_instance.expects(:absolute?).returns(true).once
-
- # we explicitly want two calls here to check that we get
- # the second from the cache
- @loader.should be_valid_search_path "/foo"
- @loader.should be_valid_search_path "/foo"
- end
-
# Used to have test for " " as a directory since that should
# be a relative directory, but on Windows in both 1.8.7 and
# 1.9.3 it is an absolute directory (WTF Windows). Considering
@@ -67,7 +45,15 @@ describe Facter::Util::Loader do
' /',
' \/',
].each do |dir|
- it "should be false for relative path #{dir}" do
+ 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
+
@loader.should_not be_valid_search_path dir
end
end
@@ -83,7 +69,15 @@ describe Facter::Util::Loader do
'/ ',
'/ /..',
].each do |dir|
- it "should be true for absolute path #{dir}" do
+ 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
+
@loader.should be_valid_search_path dir
end
end
@@ -97,8 +91,10 @@ describe Facter::Util::Loader do
end
it "should include the facter subdirectory of all paths in ruby LOAD_PATH" do
- dirs = $LOAD_PATH.collect { |d| File.join(d, "facter") }
+ dirs = $LOAD_PATH.collect { |d| File.expand_path('facter', d) }
@loader.stubs(:valid_search_path?).returns(true)
+ File.stubs(:directory?).returns true
+
paths = @loader.search_path
dirs.each do |dir|
@@ -106,15 +102,6 @@ describe Facter::Util::Loader do
end
end
- it "should warn the user when an invalid search path has been excluded" do
- dirs = $LOAD_PATH.collect { |d| File.join(d, "facter") }
- @loader.stubs(:valid_search_path?).returns(false)
- dirs.each do |dir|
- Facter.expects(:debugonce).with("Relative directory #{dir} removed from search path.").once
- end
- paths = @loader.search_path
- end
-
it "should exclude invalid search paths" do
dirs = $LOAD_PATH.collect { |d| File.join(d, "facter") }
@loader.stubs(:valid_search_path?).returns(false)
@@ -126,14 +113,28 @@ describe Facter::Util::Loader do
it "should include all search paths registered with Facter" do
Facter.expects(:search_path).returns %w{/one /two}
+ @loader.stubs(:valid_search_path?).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}
+ @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
+
+ paths = @loader.search_path
+ paths.should be_include("/one")
+ end
+
describe "and the FACTERLIB environment variable is set" do
it "should include all paths in FACTERLIB" do
Facter::Util::Resolution.with_env "FACTERLIB" => "/one/path#{File::PATH_SEPARATOR}/two/path" do
+ @loader.stubs(:valid_search_path?).returns true
paths = @loader.search_path
%w{/one/path /two/path}.each do |dir|
paths.should be_include(dir)
@@ -159,22 +160,20 @@ describe Facter::Util::Loader do
it "should load any files in the search path with names matching the fact name" do
@loader.expects(:search_path).returns %w{/one/dir /two/dir}
- FileTest.stubs(:exist?).returns false
- FileTest.expects(:exist?).with("/one/dir/testing.rb").returns true
- FileTest.expects(:exist?).with("/two/dir/testing.rb").returns true
-
+ File.stubs(:file?).returns false
+ File.expects(:file?).with("/one/dir/testing.rb").returns true
Kernel.expects(:load).with("/one/dir/testing.rb")
- Kernel.expects(:load).with("/two/dir/testing.rb")
@loader.load(:testing)
end
- it 'should load any ruby files in directories matching the fact name in the search path in sorted order regardless of the order returned by Dir.entries' do
- @loader = TestLoader.new
-
+ it 'should not load any ruby files from subdirectories matching the fact name in the search path' do
@loader.stubs(:search_path).returns %w{/one/dir}
- FileTest.stubs(:exist?).returns false
- FileTest.stubs(:directory?).with("/one/dir/testing").returns true
+ File.stubs(:file?).returns false
+ File.expects(:file?).with("/one/dir/testing.rb").returns true
+ Kernel.expects(:load).with("/one/dir/testing.rb")
+
+ File.stubs(:directory?).with("/one/dir/testing").returns true
@loader.stubs(:search_path).returns %w{/one/dir}
Dir.stubs(:entries).with("/one/dir/testing").returns %w{foo.rb bar.rb}
@@ -184,28 +183,13 @@ describe Facter::Util::Loader do
end
@loader.load(:testing)
- @loader.loaded_files.should == %w{/one/dir/testing/bar.rb /one/dir/testing/foo.rb}
- end
-
- it "should load any ruby files in directories matching the fact name in the search path" do
- @loader.expects(:search_path).returns %w{/one/dir}
- FileTest.stubs(:exist?).returns false
- FileTest.expects(:directory?).with("/one/dir/testing").returns true
-
- Dir.expects(:entries).with("/one/dir/testing").returns %w{two.rb}
-
- Kernel.expects(:load).with("/one/dir/testing/two.rb")
-
- @loader.load(:testing)
end
it "should not load files that don't end in '.rb'" do
@loader.expects(:search_path).returns %w{/one/dir}
- FileTest.stubs(:exist?).returns false
- FileTest.expects(:directory?).with("/one/dir/testing").returns true
-
- Dir.expects(:entries).with("/one/dir/testing").returns %w{one}
-
+ File.stubs(:file?).returns false
+ File.expects(:file?).with("/one/dir/testing.rb").returns false
+ File.expects(:exist?).with("/one/dir/testing").never
Kernel.expects(:load).never
@loader.load(:testing)
@@ -217,107 +201,51 @@ describe Facter::Util::Loader do
@loader = Facter::Util::Loader.new
@loader.stubs(:search_path).returns []
- FileTest.stubs(:directory?).returns true
- end
-
- it "should skip directories that do not exist" do
- @loader.expects(:search_path).returns %w{/one/dir}
-
- FileTest.expects(:directory?).with("/one/dir").returns false
-
- Dir.expects(:entries).with("/one/dir").never
-
- @loader.load_all
+ File.stubs(:directory?).returns true
end
it "should load all files in all search paths" do
@loader.expects(:search_path).returns %w{/one/dir /two/dir}
- Dir.expects(:entries).with("/one/dir").returns %w{a.rb b.rb}
- Dir.expects(:entries).with("/two/dir").returns %w{c.rb d.rb}
-
- %w{/one/dir/a.rb /one/dir/b.rb /two/dir/c.rb /two/dir/d.rb}.each { |f| Kernel.expects(:load).with(f) }
-
- @loader.load_all
- end
-
- it "should load all files in all subdirectories in all search paths" do
- @loader.expects(:search_path).returns %w{/one/dir /two/dir}
-
- Dir.expects(:entries).with("/one/dir").returns %w{a}
- Dir.expects(:entries).with("/two/dir").returns %w{b}
-
- %w{/one/dir/a /two/dir/b}.each { |f| File.expects(:directory?).with(f).returns true }
-
- Dir.expects(:entries).with("/one/dir/a").returns %w{c.rb}
- Dir.expects(:entries).with("/two/dir/b").returns %w{d.rb}
-
- %w{/one/dir/a/c.rb /two/dir/b/d.rb}.each { |f| Kernel.expects(:load).with(f) }
-
- @loader.load_all
- end
-
- it 'should load all files in sorted order for any given directory regardless of the order returned by Dir.entries' do
- @loader = TestLoader.new
-
- @loader.stubs(:search_path).returns %w{/one/dir}
- Dir.stubs(:entries).with("/one/dir").returns %w{foo.rb bar.rb}
-
- %w{/one/dir}.each { |f| File.stubs(:directory?).with(f).returns true }
+ Dir.expects(:glob).with('/one/dir/*.rb').returns %w{a.rb b.rb}
+ Dir.expects(:glob).with('/two/dir/*.rb').returns %w{c.rb d.rb}
- %w{/one/dir/foo.rb /one/dir/bar.rb}.each do |f|
- File.stubs(:directory?).with(f).returns false
+ %w{/one/dir/a.rb /one/dir/b.rb /two/dir/c.rb /two/dir/d.rb}.each do |f|
+ File.expects(:file?).with(f).returns true
Kernel.expects(:load).with(f)
end
@loader.load_all
-
- @loader.loaded_files.should == %w{/one/dir/bar.rb /one/dir/foo.rb}
- end
-
- it "should not load files in the util subdirectory" do
- @loader.expects(:search_path).returns %w{/one/dir}
-
- Dir.expects(:entries).with("/one/dir").returns %w{util}
-
- File.expects(:directory?).with("/one/dir/util").returns true
-
- Dir.expects(:entries).with("/one/dir/util").never
-
- @loader.load_all
end
- it "should not load files in a lib subdirectory" do
- @loader.expects(:search_path).returns %w{/one/dir}
-
- Dir.expects(:entries).with("/one/dir").returns %w{lib}
-
- File.expects(:directory?).with("/one/dir/lib").returns true
-
- Dir.expects(:entries).with("/one/dir/lib").never
-
- @loader.load_all
- end
-
- it "should not load files in '.' or '..'" do
- @loader.expects(:search_path).returns %w{/one/dir}
+ it "should not try to load subdirectories of search paths" do
+ @loader.expects(:search_path).returns %w{/one/dir /two/dir}
- Dir.expects(:entries).with("/one/dir").returns %w{. ..}
+ # a.rb is a directory
+ Dir.expects(:glob).with('/one/dir/*.rb').returns %w{a.rb b.rb}
+ File.expects(:file?).with('/one/dir/a.rb').returns false
+ File.expects(:file?).with('/one/dir/b.rb').returns true
+ Kernel.expects(:load).with('/one/dir/b.rb')
- File.expects(:entries).with("/one/dir/.").never
- File.expects(:entries).with("/one/dir/..").never
+ # c.rb is a directory
+ Dir.expects(:glob).with('/two/dir/*.rb').returns %w{c.rb d.rb}
+ File.expects(:file?).with('/two/dir/c.rb').returns false
+ File.expects(:file?).with('/two/dir/d.rb').returns true
+ Kernel.expects(:load).with('/two/dir/d.rb')
@loader.load_all
end
it "should not raise an exception when a file is unloadable" do
@loader.expects(:search_path).returns %w{/one/dir}
- Dir.expects(:entries).with("/one/dir").returns %w{a.rb}
+
+ Dir.expects(:glob).with('/one/dir/*.rb').returns %w{a.rb}
+ File.expects(:file?).with('/one/dir/a.rb').returns true
Kernel.expects(:load).with("/one/dir/a.rb").raises(LoadError)
Facter.expects(:warn)
- lambda { @loader.load_all }.should_not raise_error
+ expect { @loader.load_all }.to_not raise_error
end
it "should load all facts from the environment" do
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-puppet/facter.git
More information about the Pkg-puppet-devel
mailing list