From 2255abee7bdb9b6478ca228546e3d275dbac0ec3 Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Tue, 31 May 2011 13:14:18 -0700 Subject: (#7670) Never fail to find a fact that is present With the previous behavior, any fact which depended on another fact in a file not matching its name would fail to properly load the required fact, resulting in missing, incorrect, or inconsistent values. For instance, the operatingsystem fact depends on the lsbdistid fact found in lsb.rb. The first time the operatingsystem fact is requested, it requires lsb.rb, and so the required fact is loaded first. But if Facter is subsequently cleared and the operatingsystem fact requested again, the require will not occur, and the fact will not be automatically loaded because it doesn't match its filename. Now if a fact is requested and can't be found, we will attempt to load all the facts to find it. Such an approach appears heavy-handed, but in the case where we want a particular fact, this is the only way to make sure we've found it. In the case where we eventually want other facts, we are conveniently eagerly loading them. Paired-With: Jacob Helwig --- lib/facter/util/collection.rb | 6 +++++- spec/spec_helper.rb | 1 + spec/unit/interfaces_spec.rb | 7 ++----- spec/unit/memory_spec.rb | 4 +++- spec/unit/operatingsystem_spec.rb | 2 ++ spec/unit/util/loader_spec.rb | 4 ++++ 6 files changed, 17 insertions(+), 7 deletions(-) diff --git a/lib/facter/util/collection.rb b/lib/facter/util/collection.rb index 3f8e0f8..b3d3a45 100644 --- a/lib/facter/util/collection.rb +++ b/lib/facter/util/collection.rb @@ -66,9 +66,13 @@ class Facter::Util::Collection def fact(name) name = canonize(name) + # Try to load the fact if necessary loader.load(name) unless @facts[name] - return @facts[name] + # Try HARDER + loader.load_all unless @facts[name] + + @facts[name] end # Flush all cached values. diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 28e7b72..483d4dc 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -18,6 +18,7 @@ RSpec.configure do |config| # Ensure that we don't accidentally cache between test cases. config.before :each do + Facter::Util::Loader.any_instance.stubs(:load_all) Facter.clear end end diff --git a/spec/unit/interfaces_spec.rb b/spec/unit/interfaces_spec.rb index 8b295d6..cfe4226 100755 --- a/spec/unit/interfaces_spec.rb +++ b/spec/unit/interfaces_spec.rb @@ -3,17 +3,14 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') require 'facter' +require 'facter/util/ip' describe "Per Interface IP facts" do - before do - Facter.loadfacts - end - it "should replace the ':' in an interface list with '_'" do # So we look supported Facter.fact(:kernel).stubs(:value).returns("SunOS") - Facter::Util::IP.expects(:get_interfaces).returns %w{eth0:1 eth1:2} + Facter::Util::IP.stubs(:get_interfaces).returns %w{eth0:1 eth1:2} Facter.fact(:interfaces).value.should == %{eth0_1,eth1_2} end end diff --git a/spec/unit/memory_spec.rb b/spec/unit/memory_spec.rb index 5cae8cb..fe4ec36 100644 --- a/spec/unit/memory_spec.rb +++ b/spec/unit/memory_spec.rb @@ -6,7 +6,9 @@ require 'facter' describe "Memory facts" do before do - Facter.loadfacts + # We need these facts loaded, but they belong to a file with a + # different name, so load the file explicitly. + Facter.collection.loader.load(:memory) end after do diff --git a/spec/unit/operatingsystem_spec.rb b/spec/unit/operatingsystem_spec.rb index 91cd311..9a7971d 100755 --- a/spec/unit/operatingsystem_spec.rb +++ b/spec/unit/operatingsystem_spec.rb @@ -34,6 +34,7 @@ describe "Operating System fact" do it "should identify Oracle VM as OVS" do Facter.fact(:kernel).stubs(:value).returns("Linux") + Facter.stubs(:value).with(:lsbdistid).returns(nil) FileTest.stubs(:exists?).returns false FileTest.expects(:exists?).with("/etc/ovs-release").returns true @@ -44,6 +45,7 @@ describe "Operating System fact" do it "should identify VMWare ESX" do Facter.fact(:kernel).stubs(:value).returns("Linux") + Facter.stubs(:value).with(:lsbdistid).returns(nil) FileTest.stubs(:exists?).returns false FileTest.expects(:exists?).with("/etc/vmware-release").returns true diff --git a/spec/unit/util/loader_spec.rb b/spec/unit/util/loader_spec.rb index 90530e8..1bc909f 100755 --- a/spec/unit/util/loader_spec.rb +++ b/spec/unit/util/loader_spec.rb @@ -19,6 +19,10 @@ end describe Facter::Util::Loader do + before :each do + Facter::Util::Loader.any_instance.unstub(:load_all) + end + def with_env(values) old = {} values.each do |var, value| -- cgit From 926e912cd4eeedacc5833457ed34e57bd06f5b1a Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Tue, 31 May 2011 13:25:43 -0700 Subject: (#7670) Stop preloading all facts in the application If a requested fact isn't found in the same location as its name, we want to load all of the facts to find it. However, to simplify that, we were previously just preloading all the facts every time. Because requesting a fact now implicitly loads all facts if necessary, we can rely on that, providing results much more quickly in the case where facts do match their filenames. Reviewed-By: Jacob Helwig --- lib/facter/application.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/facter/application.rb b/lib/facter/application.rb index bd68149..9b6da1d 100644 --- a/lib/facter/application.rb +++ b/lib/facter/application.rb @@ -10,10 +10,6 @@ module Facter names = argv # Create the facts hash that is printed to standard out. - # Pre-load all of the facts, since we can have multiple facts - # per file, and since we can't know ahead of time which file a - # fact will be in, we'll need to load every file. - facts = Facter.to_hash unless names.empty? facts = {} names.each do |name| @@ -26,6 +22,9 @@ module Facter end end + # Print everything if they didn't ask for specific facts. + facts ||= Facter.to_hash + # Print the facts as YAML and exit if options[:yaml] require 'yaml' -- cgit From 0c23845c48488832cf224228f93f62714ba4d4ef Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Tue, 31 May 2011 14:27:16 -0700 Subject: maint: Fix spelling of acceptance directory --- .../ticket_7039_facter_multiple_facts_one_file.rb | 31 ---------------------- .../ticket_7039_facter_multiple_facts_one_file.rb | 31 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 31 deletions(-) delete mode 100644 accecptance/tests/ticket_7039_facter_multiple_facts_one_file.rb create mode 100644 acceptance/tests/ticket_7039_facter_multiple_facts_one_file.rb diff --git a/accecptance/tests/ticket_7039_facter_multiple_facts_one_file.rb b/accecptance/tests/ticket_7039_facter_multiple_facts_one_file.rb deleted file mode 100644 index fb78628..0000000 --- a/accecptance/tests/ticket_7039_facter_multiple_facts_one_file.rb +++ /dev/null @@ -1,31 +0,0 @@ -test_name "#7039: Facter having issue handling multiple facts in a single file" - -fact_file= %q{ -Facter.add(:test_fact1) do - setcode do - "test fact 1" - end -end - -Facter.add(:test_fact2) do - setcode do - "test fact 2" - end -end -} - -agent1=agents.first -step "Agent: Create fact file with multiple facts" -create_remote_file(agent1, '/tmp/test_facts.rb', fact_file ) - -step "Agent: Verify test_fact1 from /tmp/test_facts.rb" -on(agent1, "export FACTERLIB=/tmp && facter --puppet test_fact1") do - fail_test "Fact 1 not returned by facter --puppet test_fact1" unless - stdout.include? 'test fact 1' -end - -step "Agent: Verify test_fact2 from /tmp/test_facts.rb" -on(agent1, "export FACTERLIB=/tmp && facter --puppet test_fact2") do - fail_test "Fact 1 not returned by facter --puppet test_fact2" unless - stdout.include? 'test fact 2' -end diff --git a/acceptance/tests/ticket_7039_facter_multiple_facts_one_file.rb b/acceptance/tests/ticket_7039_facter_multiple_facts_one_file.rb new file mode 100644 index 0000000..fb78628 --- /dev/null +++ b/acceptance/tests/ticket_7039_facter_multiple_facts_one_file.rb @@ -0,0 +1,31 @@ +test_name "#7039: Facter having issue handling multiple facts in a single file" + +fact_file= %q{ +Facter.add(:test_fact1) do + setcode do + "test fact 1" + end +end + +Facter.add(:test_fact2) do + setcode do + "test fact 2" + end +end +} + +agent1=agents.first +step "Agent: Create fact file with multiple facts" +create_remote_file(agent1, '/tmp/test_facts.rb', fact_file ) + +step "Agent: Verify test_fact1 from /tmp/test_facts.rb" +on(agent1, "export FACTERLIB=/tmp && facter --puppet test_fact1") do + fail_test "Fact 1 not returned by facter --puppet test_fact1" unless + stdout.include? 'test fact 1' +end + +step "Agent: Verify test_fact2 from /tmp/test_facts.rb" +on(agent1, "export FACTERLIB=/tmp && facter --puppet test_fact2") do + fail_test "Fact 1 not returned by facter --puppet test_fact2" unless + stdout.include? 'test fact 2' +end -- cgit From 9404a7a3bb8da660e26897d052cdd03c291fb0bb Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Tue, 31 May 2011 14:59:20 -0700 Subject: (#7670) Add an acceptance test This test runs only on Ubuntu machines and will reproduce the problem with these steps: * load Facter * request the operatingsystem fact * clear Facter * request the operatingsystem fact and verify it is still correct Reviewed-By: Jacob Helwig --- .../operatingsystem_detection_after_clear_on_ubuntu.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 acceptance/tests/operatingsystem_detection_after_clear_on_ubuntu.rb diff --git a/acceptance/tests/operatingsystem_detection_after_clear_on_ubuntu.rb b/acceptance/tests/operatingsystem_detection_after_clear_on_ubuntu.rb new file mode 100644 index 0000000..3c7c4d9 --- /dev/null +++ b/acceptance/tests/operatingsystem_detection_after_clear_on_ubuntu.rb @@ -0,0 +1,18 @@ +test_name "#7670: Facter should properly detect operatingsystem on Ubuntu after a clear" + +script_contents = <<-OS_DETECT + require 'facter' + Facter['operatingsystem'].value + Facter.clear + exit Facter['operatingsystem'].value == 'Ubuntu' +OS_DETECT + +script_name = "/tmp/facter_os_detection_test_#{$$}" + +agents.each do |agent| + next unless agent['platform'].include? 'ubuntu' + + create_remote_file(agent, script_name, script_contents) + + on(agent, "ruby #{script_name}") +end -- cgit