Bringing this into the mailing list as Paul Nasrat suggested doing just that. Interestingly, I'm not the person who discovered the actual ordering problem (that was Michael Stahnke), but I was sitting next to him while he was working on a custom fact and some puppet manifests. The diagnosis/repair problem was interesting enough to me that I decided to give it a spin.
I think there are some interesting things being considered for Facter 2.0, but I'm also aware that the perfect is the enemy of the good -- that is, Facter < 2.0 seems to be in wide usage and the behavior this patch changes seems to be both surprising and unintentional. If it's an incremental step on the way to 2.0 then that's even better. Rick On Sun, Dec 12, 2010 at 8:10 PM, Rick Bradley <[email protected]> wrote: > Ruby's Dir.entries will return files in different orders depending on > the OS and/or filesystem. As a result Facter::Util::Loader will load > ruby custom fact definitions in different orders on different platforms. > > Specs to expose the bugs, and code to ensure that custom fact files are > loaded in alphabetical order. > > Addresses redmine issue #5510 > > http://projects.puppetlabs.com/issues/5510 > > Signed-off-by: Rick Bradley <[email protected]> > --- > Local-branch: ticket/master/5510 > lib/facter/util/loader.rb | 4 ++-- > spec/unit/util/loader.rb | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+), 2 deletions(-) > > diff --git a/lib/facter/util/loader.rb b/lib/facter/util/loader.rb > index ac90c48..b0f43dd 100644 > --- a/lib/facter/util/loader.rb > +++ b/lib/facter/util/loader.rb > @@ -30,7 +30,7 @@ class Facter::Util::Loader > search_path.each do |dir| > next unless FileTest.directory?(dir) > > - Dir.entries(dir).each do |file| > + Dir.entries(dir).sort.each do |file| > path = File.join(dir, file) > if File.directory?(path) > load_dir(path) > @@ -62,7 +62,7 @@ class Facter::Util::Loader > def load_dir(dir) > return if dir =~ /\/\.+$/ or dir =~ /\/util$/ or dir =~ /\/lib$/ > > - Dir.entries(dir).find_all { |f| f =~ /\.rb$/ }.each do |file| > + Dir.entries(dir).sort.find_all { |f| f =~ /\.rb$/ }.each do |file| > load_file(File.join(dir, file)) > end > end > diff --git a/spec/unit/util/loader.rb b/spec/unit/util/loader.rb > index 0a28020..ebd76ea 100755 > --- a/spec/unit/util/loader.rb > +++ b/spec/unit/util/loader.rb > @@ -4,6 +4,19 @@ require File.dirname(__FILE__) + '/../../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 > def with_env(values) > old = {} > @@ -107,6 +120,21 @@ describe Facter::Util::Loader do > > @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 > + > + �[email protected](:search_path).returns %w{/one/dir} > + FileTest.stubs(:exist?).returns false > + FileTest.stubs(:directory?).with("/one/dir/testing").returns true > + �[email protected](:search_path).returns %w{/one/dir} > + > + Dir.stubs(:entries).with("/one/dir/testing").returns %w{foo.rb > bar.rb} > + %w{/one/dir/testing/foo.rb /one/dir/testing/bar.rb}.each { |f| > File.stubs(:directory?).with(f).returns false } > + > + �[email protected](:testing) > + �[email protected]_files.should == %w{/one/dir/testing/bar.rb > /one/dir/testing/foo.rb} > + end > > it "should not load files that don't end in '.rb'" do > @loader.expects(:search_path).returns %w{/one/dir} > @@ -165,6 +193,20 @@ describe Facter::Util::Loader do > > @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 > + > + �[email protected](: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 } > + %w{/one/dir/foo.rb /one/dir/bar.rb}.each { |f| > File.stubs(:directory?).with(f).returns false } > + > + �[email protected]_all > + > + �[email protected]_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} > -- > 1.7.0.2 > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
