2nd round to fix #2766

[Peter Meier]

Hi

these two patches are a second round to fix #2677 I have addressed your
comments regarding tests and introduced a global warn mechanism to send
output to Kernel.warn if debugging is enabled.

cheers pete

[Peter Meier]

We can now warn messages that will be passed to Kernel.warn if
debugging is enabled.

Signed-off-by: Peter Meier <peter...@immerda.ch>
---
lib/facter.rb | 13 ++++++++-
spec/unit/facter.rb | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 1 deletions(-)

diff --git a/lib/facter.rb b/lib/facter.rb
index 3c69a94..ac06f68 100644
--- a/lib/facter.rb
+++ b/lib/facter.rb
@@ -68,11 +68,15 @@ module Facter
if string.nil?
return
end
- if @@debug != 0
+ if self.debugging?
puts GREEN + string + RESET
end
end

+ def self.debugging?
+ @@debug != 0
+ end
+
# Return a fact object by name. If you use this, you still have to call
# 'value' on it to retrieve the actual value.
def self.[](name)
@@ -173,6 +177,13 @@ module Facter
end
end

+
+ def self.warn(msg)
+ if Facter.debugging? and msg and not msg.empty?
+ msg.each { |line| Kernel.warn line }
+ end
+ end
+
# Remove them all.
def self.reset
@collection = nil
diff --git a/spec/unit/facter.rb b/spec/unit/facter.rb
index f248aa7..da23ee7 100755
--- a/spec/unit/facter.rb
+++ b/spec/unit/facter.rb
@@ -131,6 +131,79 @@ describe Facter do
Facter.should respond_to(:search_path)
end

+ it "should have a method to query debugging mode" do
+ Facter.should respond_to(:debugging?)
+ end
+
+ it "should have a method to warn" do
+ Facter.should respond_to(:warn)
+ end
+
+ describe "when warning" do
+ it "should warn if debugging is enabled" do
+ Facter.debugging(true)
+ Kernel.stubs(:warn)
+ Kernel.expects(:warn).with('foo')
+ Facter.warn('foo')
+ end
+
+ it "should not warn if debugging is enabled but nil is passed" do
+ Facter.debugging(true)
+ Kernel.stubs(:warn)
+ Kernel.expects(:warn).never
+ Facter.warn(nil)
+ end
+
+ it "should not warn if debugging is enabled but an empyt string is passed" do
+ Facter.debugging(true)
+ Kernel.stubs(:warn)
+ Kernel.expects(:warn).never
+ Facter.warn('')
+ end
+
+ it "should not warn if debugging is disabled" do
+ Facter.debugging(false)
+ Kernel.stubs(:warn)
+ Kernel.expects(:warn).never
+ Facter.warn('foo')
+ end
+
+ it "should warn for any given element for an array if debugging is enabled" do
+ Facter.debugging(true)
+ Kernel.stubs(:warn)
+ Kernel.expects(:warn).with('foo')
+ Kernel.expects(:warn).with('bar')
+ Facter.warn( ['foo','bar'])
+ end
+ end
+
+ describe "when setting debugging mode" do
+ it "should have debugging enabled using 1" do
+ Facter.debugging(1)
+ Facter.debugging?.should == true
+ end
+ it "should have debugging enabled using true" do
+ Facter.debugging(true)
+ Facter.debugging?.should == true
+ end
+ it "should have debugging enabled using any string except off" do
+ Facter.debugging('aaaaa')
+ Facter.debugging?.should == true
+ end
+ it "should have debugging disabled using 0" do
+ Facter.debugging(0)
+ Facter.debugging?.should == false
+ end
+ it "should have debugging disabled using false" do
+ Facter.debugging(false)
+ Facter.debugging?.should == false
+ end
+ it "should have debugging disabled using the string 'off'" do
+ Facter.debugging('off')
+ Facter.debugging?.should == false
+ end
+ end
+
describe "when registering directories to search" do
after { Facter.instance_variable_set("@search_path", []) }

--
1.6.3.3

[Peter Meier]

So far messages to stderr haven't been catched by
Facter::Util::Resolution.exec and were insted printed out to
stderr. This will cause facter and even puppet to print to stderr
themself, which is not very nice when running puppetd by cron,
as you might get every run a mail if a command outputs to stderr.

We are now wrapping the command execution with Open3.popen3 to
catch stderr and passing them to the new introduced Facter.warn
method.

We are also catching multiline outputs chomping newlines and
returning an array if there have been more than one line. Otherwise
we return an array containing the different lines.

This prevents in general cases as described in #2766 and should
handle command execution in a bit saner way.

Signed-off-by: Peter Meier <peter...@immerda.ch>
---

lib/facter/util/resolution.rb | 23 +++++++++++++++-----
spec/unit/util/resolution.rb | 44 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index b9e28e8..f6afce6 100644
--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -7,6 +7,7 @@ require 'facter/util/confine'

require 'timeout'
require 'rbconfig'
+require 'open3'

class Facter::Util::Resolution
attr_accessor :interpreter, :code, :name, :timeout
@@ -42,17 +43,27 @@ class Facter::Util::Resolution
end

out = nil
+ err = nil
begin
- out = %x{#{code}}.chomp
+ Open3.popen3(code) do |stdin,stdout,stderr|
+ out = self.parse_output(stdout.readlines)
+ err = self.parse_output(stderr.readlines)
+ end
rescue => detail
$stderr.puts detail
return nil
end
- if out == ""
- return nil
- else
- return out
- end
+ Facter.warn(err)
+
+ return nil if out == ""
+ out
+ end
+
+ def self.parse_output(output)
+ return nil unless output and output.size > 0
+ result = output.collect{|line| line.chomp }
+ return result.first unless result.size > 1
+ result
end

# Add a new confine to the resolution mechanism.
diff --git a/spec/unit/util/resolution.rb b/spec/unit/util/resolution.rb
index d4bb781..fcdc04c 100755
--- a/spec/unit/util/resolution.rb
+++ b/spec/unit/util/resolution.rb
@@ -227,10 +227,54 @@ describe Facter::Util::Resolution do
Facter::Util::Resolution.should respond_to(:exec)
end

+ it "should have a class method to parse output" do
+ Facter::Util::Resolution.should respond_to(:parse_output)
+ end
+
# It's not possible, AFAICT, to mock %x{}, so I can't really test this bit.
describe "when executing code" do
it "should fail if any interpreter other than /bin/sh is requested" do
lambda { Facter::Util::Resolution.exec("/something", "/bin/perl") }.should raise_error(ArgumentError)
end
+
+ it "should produce stderr content as a warning" do
+ stdout = stdin = stub('fh', :readlines => ["aaa\n"])
+ stderr = stub('stderr', :readlines => %w{my content})
+ Open3.expects(:popen3).with("/bin/true").yields(stdin, stdout, stderr)
+
+ Facter.expects(:warn).with(['my','content'])
+ Facter::Util::Resolution.exec("/bin/true").should == 'aaa'
+ end
+
+ it "should produce nil as a warning if nothing is printed to stderr" do
+ stdout = stdin = stub('fh', :readlines => ["aaa\n"])
+ stderr = stub('stderr', :readlines => [])
+ Open3.expects(:popen3).with("/bin/true").yields(stdin, stdout, stderr)
+
+ Facter.expects(:warn).with(nil)
+ Facter::Util::Resolution.exec("/bin/true").should == 'aaa'

+ end
+ end
+

+ describe "when parsing output" do
+ it "should return nil on nil" do
+ Facter::Util::Resolution.parse_output(nil).should be_nil
+ end
+
+ it "should return nil on empty string" do
+ Facter::Util::Resolution.parse_output('').should be_nil
+ end
+
+ it "should return nil on an empty array" do
+ Facter::Util::Resolution.parse_output([]).should be_nil
+ end
+
+ it "should return a string on a 1 size array" do
+ Facter::Util::Resolution.parse_output(["aaa\n"]).should == "aaa"
+ end
+
+ it "should return a string on a multi sized array containing a seperator" do
+ result = Facter::Util::Resolution.parse_output(["aaa\n","bbb\n","ccc\n"]).should == [ "aaa", "bbb", "ccc" ]
+ end
end
end
--
1.6.3.3

[Luke Kanies]

+1

--
God loved the birds and invented trees. Man loved the birds and
invented cages. -- Jacques Deval
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Luke Kanies]

+1, but comment below

On Nov 6, 2009, at 4:02 AM, Peter Meier wrote:

>

This can be more elegantly done as:

Facter.should be_debugging

--
Hollywood is a place where they'll pay you a thousand dollars for a
kiss and fifty cents for your soul. -- Marilyn Monroe

[Peter Meier]

>> + describe "when setting debugging mode" do
>> + it "should have debugging enabled using 1" do
>> + Facter.debugging(1)
>> + Facter.debugging?.should == true
>> + end
>
> This can be more elegantly done as:
>
> Facter.should be_debugging

ok I'll change that. Is it ok, that I'll change that and put a branch
online.

cheers pete

[Peter Meier]

http://github.com/duritong/facter/commit/71602daf684f944c75a09943682735400adf9d7d#L1R183

branch up @ http://github.com/duritong/facter/commits/bug/master/2766

ticket updated.

cheers pete