Please review pull request #636: Refactor/master/12396 dry up timeout opened by (cprice-puppet)
Description:
This is just an update to pull request #450 to address the comments there from dpittman and nlew.
- Opened: Fri Apr 06 17:50:47 UTC 2012
- Based on: puppetlabs:master (d8a0ab06ac15eea479f17732e0792f994d05ccfc)
- Requested merge: cprice-puppet:refactor/master/12396-dry-up-timeout (ddbb984380cfeacdb006a7f1661d2b4e534293d3)
Diff follows:
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index a6058b8..b34e9dd 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -3,11 +3,13 @@
require 'timeout'
require 'puppet/network/http_pool'
require 'puppet/util'
+require 'puppet/util/config_timeout'
class Puppet::Configurer
require 'puppet/configurer/fact_handler'
require 'puppet/configurer/plugin_handler'
+ extend Puppet::Util::ConfigTimeout
include Puppet::Configurer::FactHandler
include Puppet::Configurer::PluginHandler
@@ -201,23 +203,6 @@ def save_last_run_summary(report)
private
- def self.timeout
- timeout = Puppet[:configtimeout]
- case timeout
- when String
- if timeout =~ /^\d+$/
- timeout = Integer(timeout)
- else
- raise ArgumentError, "Configuration timeout must be an integer"
- end
- when Integer # nothing
- else
- raise ArgumentError, "Configuration timeout must be an integer"
- end
-
- timeout
- end
-
def execute_from_setting(setting)
return true if (command = Puppet[setting]) == ""
diff --git a/lib/puppet/configurer/downloader.rb b/lib/puppet/configurer/downloader.rb
index 4a70c06..4814ce3 100644
--- a/lib/puppet/configurer/downloader.rb
+++ b/lib/puppet/configurer/downloader.rb
@@ -1,34 +1,19 @@
require 'puppet/configurer'
require 'puppet/resource/catalog'
+require 'puppet/util/config_timeout'
class Puppet::Configurer::Downloader
+ extend Puppet::Util::ConfigTimeout
+
attr_reader :name, :path, :source, :ignore
- # Determine the timeout value to use.
- def self.timeout
- timeout = Puppet[:configtimeout]
- case timeout
- when String
- if timeout =~ /^\d+$/
- timeout = Integer(timeout)
- else
- raise ArgumentError, "Configuration timeout must be an integer"
- end
- when Integer # nothing
- else
- raise ArgumentError, "Configuration timeout must be an integer"
- end
-
- timeout
- end
-
# Evaluate our download, returning the list of changed values.
def evaluate
Puppet.info "Retrieving #{name}"
files = []
begin
- Timeout.timeout(self.class.timeout) do
+ ::Timeout.timeout(self.class.timeout_interval) do
catalog.apply do |trans|
trans.changed?.find_all do |resource|
yield resource if block_given?
diff --git a/lib/puppet/configurer/fact_handler.rb b/lib/puppet/configurer/fact_handler.rb
index 6027732..9a319d5 100644
--- a/lib/puppet/configurer/fact_handler.rb
+++ b/lib/puppet/configurer/fact_handler.rb
@@ -1,5 +1,6 @@
require 'puppet/indirector/facts/facter'
+require 'puppet/configurer'
require 'puppet/configurer/downloader'
# Break out the code related to facts. This module is
diff --git a/lib/puppet/indirector/facts/facter.rb b/lib/puppet/indirector/facts/facter.rb
index e41ef02..99fab97 100644
--- a/lib/puppet/indirector/facts/facter.rb
+++ b/lib/puppet/indirector/facts/facter.rb
@@ -1,7 +1,10 @@
require 'puppet/node/facts'
require 'puppet/indirector/code'
+require 'puppet/util/config_timeout'
class Puppet::Node::Facts::Facter < Puppet::Indirector::Code
+ extend Puppet::Util::ConfigTimeout
+
desc "Retrieve facts from Facter. This provides a somewhat abstract interface
between Puppet and Facter. It's only `somewhat` abstract because it always
returns the local host's facts, regardless of what you attempt to find."
@@ -44,7 +47,7 @@ def self.load_facts_in_dir(dir)
fqfile = ::File.join(dir, file)
begin
Puppet.info "Loading facts in #{fqfile}"
- Timeout::timeout(self.timeout) do
+ ::Timeout::timeout(self.timeout_interval) do
load file
end
rescue SystemExit,NoMemoryError
@@ -56,23 +59,6 @@ def self.load_facts_in_dir(dir)
end
end
- def self.timeout
- timeout = Puppet[:configtimeout]
- case timeout
- when String
- if timeout =~ /^\d+$/
- timeout = Integer(timeout)
- else
- raise ArgumentError, "Configuration timeout must be an integer"
- end
- when Integer # nothing
- else
- raise ArgumentError, "Configuration timeout must be an integer"
- end
-
- timeout
- end
-
def destroy(facts)
raise Puppet::DevError, "You cannot destroy facts in the code store; it is only used for getting facts from Facter"
end
diff --git a/lib/puppet/util/config_timeout.rb b/lib/puppet/util/config_timeout.rb
new file mode 100644
index 0000000..5da6507
--- /dev/null
+++ b/lib/puppet/util/config_timeout.rb
@@ -0,0 +1,24 @@
+module Puppet::Util::ConfigTimeout
+ # NOTE: in the future it might be a good idea to add an explicit "integer" type to
+ # the settings types, in which case this would no longer be necessary.
+
+ # Get the value of puppet's "configtimeout" setting, as an integer. Raise an
+ # ArgumentError if the setting does not contain a valid integer value.
+ # @return Puppet config timeout setting value, as an integer
+ def timeout_interval
+ timeout = Puppet[:configtimeout]
+ case timeout
+ when String
+ if timeout =~ /^\d+$/
+ timeout = Integer(timeout)
+ else
+ raise ArgumentError, "Configuration timeout must be an integer"
+ end
+ when Integer # nothing
+ else
+ raise ArgumentError, "Configuration timeout must be an integer"
+ end
+
+ timeout
+ end
+end
\ No newline at end of file
diff --git a/spec/unit/configurer/downloader_spec.rb b/spec/unit/configurer/downloader_spec.rb
index 439a25f..9dba85b 100755
--- a/spec/unit/configurer/downloader_spec.rb
+++ b/spec/unit/configurer/downloader_spec.rb
@@ -22,12 +22,12 @@
end
it "should be able to provide a timeout value" do
- Puppet::Configurer::Downloader.should respond_to(:timeout)
+ Puppet::Configurer::Downloader.should respond_to(:timeout_interval)
end
it "should use the configtimeout, converted to an integer, as its timeout" do
Puppet.settings.expects(:value).with(:configtimeout).returns "50"
- Puppet::Configurer::Downloader.timeout.should == 50
+ Puppet::Configurer::Downloader.timeout_interval.should == 50
end
describe "when creating the file that does the downloading" do
@@ -155,7 +155,7 @@
end
it "should set a timeout for the download" do
- Puppet::Configurer::Downloader.expects(:timeout).returns 50
+ Puppet::Configurer::Downloader.expects(:timeout_interval).returns 50
Timeout.expects(:timeout).with(50)
@dler.evaluate
diff --git a/spec/unit/provider/README.markdown b/spec/unit/provider/README.markdown
new file mode 100644
index 0000000..7025850
--- /dev/null
+++ b/spec/unit/provider/README.markdown
@@ -0,0 +1,4 @@
+Provider Specs
+==============
+
+Define specs for your providers under this directory.
diff --git a/spec/unit/puppet/provider/README.markdown b/spec/unit/puppet/provider/README.markdown
deleted file mode 100644
index 7025850..0000000
--- a/spec/unit/puppet/provider/README.markdown
+++ /dev/null
@@ -1,4 +0,0 @@
-Provider Specs
-==============
-
-Define specs for your providers under this directory.
diff --git a/spec/unit/puppet/type/README.markdown b/spec/unit/puppet/type/README.markdown
deleted file mode 100644
index 1ee19ac..0000000
--- a/spec/unit/puppet/type/README.markdown
+++ /dev/null
@@ -1,4 +0,0 @@
-Resource Type Specs
-===================
-
-Define specs for your resource types in this directory.
diff --git a/spec/unit/type/README.markdown b/spec/unit/type/README.markdown
new file mode 100644
index 0000000..1ee19ac
--- /dev/null
+++ b/spec/unit/type/README.markdown
@@ -0,0 +1,4 @@
+Resource Type Specs
+===================
+
+Define specs for your resource types in this directory.
diff --git a/spec/unit/util/config_timeout_spec.rb b/spec/unit/util/config_timeout_spec.rb
new file mode 100644
index 0000000..bada38c
--- /dev/null
+++ b/spec/unit/util/config_timeout_spec.rb
@@ -0,0 +1,57 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+require 'puppet/util/config_timeout'
+
+
+
+describe Puppet::Util::ConfigTimeout do
+ # NOTE: in the future it might be a good idea to add an explicit "integer" type to
+ # the settings types, in which case this would no longer be necessary.
+
+
+ class TestConfigTimeout
+ include Puppet::Util::ConfigTimeout
+ end
+
+ let :instance do TestConfigTimeout.new end
+
+
+ context "when the config setting is a String" do
+ context "which contains an integer" do
+ it "should convert the string to an integer" do
+ Puppet[:configtimeout] = "12"
+ instance.timeout_interval.should == 12
+ end
+ end
+
+ context "which does not contain an integer do" do
+ it "should raise an ArgumentError" do
+ Puppet[:configtimeout] = "foo"
+ expect {
+ instance.timeout_interval
+ }.to raise_error(ArgumentError)
+ end
+ end
+ end
+
+ context "when the config setting is an Integer" do
+ it "should return the integer" do
+ Puppet[:configtimeout] = 12
+ instance.timeout_interval.should == 12
+ end
+ end
+
+ context "when the config setting is some other type" do
+ # test a random smattering of types
+ [Hash.new, Array.new, Object.new].each do |obj|
+ context "when the config setting is a #{obj.class}" do
+ it "should raise an ArgumentError" do
+ Puppet[:configtimeout] = Hash.new
+ expect {
+ instance.timeout_interval
+ }.to raise_error(ArgumentError)
+ end
+ end
+ end
+ end
+end
\ No newline at end of file
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
