Please review pull request #702: (#13948) $libdir not in $LOAD_PATH opened by (jeffweiss)
Description:
Change :call_on_define to :call_hook with values of:on_define_and_write, :on_initialize_and_write, and :on_write_only.
Make hook for $libdir :on_initialize_and_write.
Make hook for $factpath :on_initialize_and_write.
Make hook for $storeconfigs :on_initialize_and_write.
Change Puppet::Util::Settings to call hooks on settings designated as:on_initialize_and_write immediately after initialing default
application values.
Add has_hook? convenience method toPuppet::Util::Settings::StringSetting.Puppet::Util::Settings::StringSetting#call_on_define
Deprecate.Puppet::Util::Settings::StringSetting` for
Add convenience methods to
checking if hook should be called at definition or at initialization.
Add tests for above changes.
Prior to this commit, the hooks for $libdir and $factpath were not
getting called unless explicitly overridden in the code. This meant that
the default $libdir was not part of $LOAD_PATH and the default $factpath
was not searched for facts. This commit fixes those problems by
enabling settings to declare when the hook should be called. A hook can
be only only when a value is overwritten (:on_write_only), which is the
default. A hook can be called with the default value when when the
setting is defined (as soon as Puppet::Util::Settings.define_settings is
called) (:on_define_and_write), in addition to any time the value is
overwritten. A hook can be called with the default value during
application initialization (:on_initialize_and_write), in addition to
any time the value is overwritten.
- Opened: Mon Apr 23 22:09:09 UTC 2012
- Based on: puppetlabs:master (54e1c83afde1b721e5433adea17a4dd0caffbc63)
- Requested merge: jeffweiss:ticket/master/13948_libdir_not_in_load_path (093e404f54eb6edfef063897fef5161f5b3230fb)
Diff follows:
diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index b579829..1432d9d 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -136,7 +136,7 @@ module Puppet
:default => "none",
:desc => "The shell search path. Defaults to whatever is inherited\n" +
"from the parent process.",
- :call_on_define => true, # Call our hook with the default value, so we always get the libdir set.
+ :call_hook => :on_define_and_write,
:hook => proc do |value|
ENV["PATH"] = "" if ENV["PATH"].nil?
ENV["PATH"] = value unless value == "none"
@@ -155,7 +155,7 @@ module Puppet
"guaranteed to work for those cases. In fact, the autoload\n" +
"mechanism is responsible for making sure this directory\n" +
"is in Ruby's search path\n",
- #:call_on_define => true, # Call our hook with the default value, so we always get the libdir set.
+ :call_hook => :on_initialize_and_write,
:hook => proc do |value|
$LOAD_PATH.delete(@oldlibdir) if defined?(@oldlibdir) and $LOAD_PATH.include?(@oldlibdir)
@oldlibdir = value
@@ -370,7 +370,7 @@ module Puppet
:certname => {
:default => fqdn.downcase, :desc => "The name to use when handling certificates. Defaults
to the fully qualified domain name.",
- :call_on_define => true, # Call our hook with the default value, so we're always downcased
+ :call_hook => :on_define_and_write, # Call our hook with the default value, so we're always downcased
:hook => proc { |value| raise(ArgumentError, "Certificate names must be lower case; see #1168") unless value == value.downcase }},
:certdnsnames => {
:default => '',
@@ -692,7 +692,7 @@ module Puppet
a proxy in front of the process or processes, since Mongrel cannot
speak SSL.",
- :call_on_define => true, # Call our hook with the default value, so we always get the correct bind address set.
+ :call_hook => :on_define_and_write, # Call our hook with the default value, so we always get the correct bind address set.
:hook => proc { |value| value == "webrick" ? Puppet.settings[:bindaddress] = "0.0.0.0" : Puppet.settings[:bindaddress] = "127.0.0.1" if Puppet.settings[:bindaddress] == "" }
}
)
@@ -1083,7 +1083,7 @@ module Puppet
},
:reportserver => {
:default => "$server",
- :call_on_define => false,
+ :call_hook => :on_write_only,
:desc => "(Deprecated for 'report_server') The server to which to send transaction reports.",
:hook => proc do |value|
Puppet.settings[:report_server] = value if value
@@ -1202,7 +1202,7 @@ module Puppet
:desc => "Where Puppet should look for facts. Multiple directories should
be separated by the system path separator character. (The POSIX path separator is ':', and the Windows path separator is ';'.)",
- #:call_on_define => true, # Call our hook with the default value, so we always get the value added to facter.
+ :call_hook => :on_initialize_and_write, # Call our hook with the default value, so we always get the value added to facter.
:hook => proc { |value| Facter.search(value) if Facter.respond_to?(:search) }}
)
@@ -1434,7 +1434,7 @@ module Puppet
You can adjust the backend using the storeconfigs_backend setting.",
# Call our hook with the default value, so we always get the libdir set.
- #:call_on_define => true,
+ :call_hook => :on_initialize_and_write,
:hook => proc do |value|
require 'puppet/node'
require 'puppet/node/facts'
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index d6b54a3..7fe8d82 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -127,11 +127,15 @@ def initialize_app_defaults(app_defaults)
app_defaults.each do |key, value|
set_value(key, value, :application_defaults)
end
-
+ call_hooks_deferred_to_application_initialization
@app_defaults_initialized = true
end
+ def call_hooks_deferred_to_application_initialization
+ @hooks_to_call_on_application_initialization.each { |setting| setting.handle(self.value(setting.name)) }
+ end
+
# Do variable interpolation on the value.
def convert(value, environment = nil)
return nil if value.nil?
@@ -244,6 +248,8 @@ def initialize
# The list of sections we've used.
@used = []
+
+ @hooks_to_call_on_application_initialization = []
end
# NOTE: ACS ahh the util classes. . .sigh
@@ -644,7 +650,7 @@ def set_value(param, value, type, options = {})
end
value = setting.munge(value) if setting.respond_to?(:munge)
- setting.handle(value) if setting.respond_to?(:handle) and not options[:dont_trigger_handles]
+ setting.handle(value) if setting.has_hook? and not options[:dont_trigger_handles]
if ReadOnly.include? param and type != :application_defaults
raise ArgumentError,
"You're attempting to set configuration parameter $#{param}, which is read-only."
@@ -741,7 +747,8 @@ def define_settings(section, defs)
# Collect the settings that need to have their hooks called immediately.
# We have to collect them so that we can be sure we're fully initialized before
# the hook is called.
- call << tryconfig if tryconfig.call_on_define
+ call << tryconfig if tryconfig.call_hook_on_define?
+ @hooks_to_call_on_application_initialization << tryconfig if tryconfig.call_hook_on_initialize?
}
call.each { |setting| setting.handle(self.value(setting.name)) }
@@ -1013,7 +1020,7 @@ def each_source(environment)
# Return all settings that have associated hooks; this is so
# we can call them after parsing the configuration file.
def settings_with_hooks
- @config.values.find_all { |setting| setting.respond_to?(:handle) }
+ @config.values.find_all { |setting| setting.has_hook? }
end
# Extract extra setting information for files.
diff --git a/lib/puppet/util/settings/string_setting.rb b/lib/puppet/util/settings/string_setting.rb
index 7405952..d035950 100644
--- a/lib/puppet/util/settings/string_setting.rb
+++ b/lib/puppet/util/settings/string_setting.rb
@@ -1,12 +1,48 @@
# The base element type.
class Puppet::Util::Settings::StringSetting
- attr_accessor :name, :section, :default, :call_on_define
+ attr_accessor :name, :section, :default, :call_on_define, :call_hook
attr_reader :desc, :short
+ def self.available_call_hook_values
+ [:on_define_and_write, :on_initialize_and_write, :on_write_only]
+ end
+
def desc=(value)
@desc = value.gsub(/^\s*/, '')
end
+ def call_on_define
+ Puppet.deprecation_warning "call_on_define has been deprecated. Please use call_hook_on_define?"
+ call_hook_on_define?
+ end
+
+ def call_on_define=(value)
+ if value
+ Puppet.deprecation_warning ":call_on_define has been changed to :call_hook => :on_define_and_write. Please change #{name}."
+ @call_hook = :on_define_and_write
+ else
+ Puppet.deprecation_warning ":call_on_define => :false has been changed to :call_hook => :on_write_only. Please change #{name}."
+ @call_hook = :on_write_only
+ end
+ end
+
+ def call_hook=(value)
+ if value.nil?
+ Puppet.warning "Setting :#{name} :call_hook is nil, defaulting to :on_write_only"
+ value ||= :on_write_only
+ end
+ raise ArgumentError, "Invalid option #{value} for call_hook" unless self.class.available_call_hook_values.include? value
+ @call_hook = value
+ end
+
+ def call_hook_on_define?
+ call_hook == :on_define_and_write
+ end
+
+ def call_hook_on_initialize?
+ call_hook == :on_initialize_and_write
+ end
+
#added as a proper method, only to generate a deprecation warning
#and return value from
def setbycli
@@ -38,6 +74,10 @@ def optparse_args
end
end
+ def has_hook?
+ respond_to? :handle
+ end
+
def hook=(block)
meta_def :handle, &block
end
@@ -48,6 +88,15 @@ def initialize(args = {})
raise ArgumentError.new("You must refer to a settings object")
end
+ # explicitly set name prior to calling other param= methods to provide meaningful feedback during
+ # other warnings
+ @name = args[:name] if args.include? :name
+
+ #set the default value for call_hook
+ @call_hook = :on_write_only if args[:hook] and not args[:call_hook]
+
+ raise ArgumentError, "Cannot reference :call_hook if no :hook is defined" if args[:call_hook] and not args[:hook]
+
args.each do |param, value|
method = param.to_s + "="
raise ArgumentError, "#{self.class} (setting '#{args[:name]}') does not accept #{param}" unless self.respond_to? method
diff --git a/spec/unit/util/settings_spec.rb b/spec/unit/util/settings_spec.rb
index d8b3af2..54e4634 100755
--- a/spec/unit/util/settings_spec.rb
+++ b/spec/unit/util/settings_spec.rb
@@ -96,6 +96,8 @@
@settings.expects(:run_mode=).with("foo")
@settings.initialize_app_defaults(app_defaults)
end
+
+ it "should call hooks on values that have been deferred to application initialization"
end
describe "when setting values" do
@@ -216,6 +218,110 @@
@settings[:myval] = "yay"
end
+ describe "call_hook" do
+ Puppet::Util::Settings::StringSetting.available_call_hook_values.each do |val|
+ describe "when :#{val}" do
+ describe "and definition invalid" do
+ it "should raise error if no hook defined" do
+ expect do
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => val})
+ end.to raise_error ArgumentError, /no :hook/
+ end
+ end
+ describe "and definition valid" do
+ before(:each) do
+ hook_values = []
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => val, :hook => lambda { |v| hook_values << v }})
+ end
+
+ it "should call the hook when value written" do
+ @settings.setting(:hooker).expects(:handle).with("something").once
+ @settings[:hooker] = "something"
+ end
+ end
+ end
+ end
+
+ it "should have a default value of :on_write_only" do
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :hook => lambda { |v| hook_values << v }})
+ @settings.setting(:hooker).call_hook.should == :on_write_only
+ end
+
+ describe "when nil" do
+ it "should generate a warning" do
+ Puppet.expects(:warning)
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => nil, :hook => lambda { |v| hook_values << v }})
+ end
+ it "should use default" do
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => nil, :hook => lambda { |v| hook_values << v }})
+ @settings.setting(:hooker).call_hook.should == :on_write_only
+ end
+ end
+
+ describe "when invalid" do
+ it "should raise an error" do
+ expect do
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => :foo, :hook => lambda { |v| hook_values << v }})
+ end.to raise_error ArgumentError, /invalid.*call_hook/i
+ end
+ end
+
+ describe "when :on_define_and_write" do
+ it "should call the hook at definition" do
+ hook_values = []
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => :on_define_and_write, :hook => lambda { |v| hook_values << v }})
+ @settings.setting(:hooker).call_hook.should == :on_define_and_write
+ hook_values.should == %w{yay}
+ end
+ end
+
+ describe "when :on_initialize_and_write" do
+ before(:each) do
+ @hook_values = []
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => :on_initialize_and_write, :hook => lambda { |v| @hook_values << v }})
+ end
+
+ it "should not call the hook at definition" do
+ @hook_values.should == []
+ @hook_values.should_not == %w{yay}
+ end
+
+ it "should call the hook at initialization" do
+ app_defaults = {}
+ Puppet::Util::Settings::REQUIRED_APP_SETTINGS.each do |key|
+ app_defaults[key] = "foo"
+ end
+ app_defaults[:run_mode] = :user
+ @settings.define_settings(:main, PuppetSpec::Settings::TEST_APP_DEFAULT_DEFINITIONS)
+
+ @settings.setting(:hooker).expects(:handle).with("yay").once
+
+ @settings.initialize_app_defaults app_defaults
+ end
+ end
+ end
+
+ describe "call_on_define" do
+ [true, false].each do |val|
+ describe "to #{val}" do
+ it "should generate a deprecation warning" do
+ Puppet.expects(:deprecation_warning)
+ values = []
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_on_define => val, :hook => lambda { |v| values << v }})
+ end
+
+ it "should should set call_hook" do
+ values = []
+ name = "hooker_#{val}".to_sym
+ @settings.define_settings(:section, name => {:default => "yay", :desc => "boo", :call_on_define => val, :hook => lambda { |v| values << v }})
+
+ @settings.setting(name).call_hook.should == :on_define_and_write if val
+ @settings.setting(name).call_hook.should == :on_write_only unless val
+ end
+ end
+ end
+ end
+
it "should call passed blocks when values are set" do
values = []
@settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :hook => lambda { |v| values << v }})
@@ -237,14 +343,14 @@
it "should provide an option to call passed blocks during definition" do
values = []
- @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_on_define => true, :hook => lambda { |v| values << v }})
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => :on_define_and_write, :hook => lambda { |v| values << v }})
values.should == %w{yay}
end
it "should pass the fully interpolated value to the hook when called on definition" do
values = []
@settings.define_settings(:section, :_one_ => { :default => "test", :desc => "a" })
- @settings.define_settings(:section, :hooker => {:default => "$one/yay", :desc => "boo", :call_on_define => true, :hook => lambda { |v| values << v }})
+ @settings.define_settings(:section, :hooker => {:default => "$one/yay", :desc => "boo", :call_hook => :on_define_and_write, :hook => lambda { |v| values << v }})
values.should == %w{test/yay}
end
@@ -301,6 +407,22 @@
FileTest.stubs(:exist?).returns true
end
+ describe "call_on_define" do
+ it "should generate a deprecation warning" do
+ Puppet.expects(:deprecation_warning)
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :hook => lambda { |v| hook_values << v }})
+ @settings.setting(:hooker).call_on_define
+ end
+
+ Puppet::Util::Settings::StringSetting.available_call_hook_values.each do |val|
+ it "should match value for call_hook => :#{val}" do
+ hook_values = []
+ @settings.define_settings(:section, :hooker => {:default => "yay", :desc => "boo", :call_hook => val, :hook => lambda { |v| hook_values << v }})
+ @settings.setting(:hooker).call_on_define.should == @settings.setting(:hooker).call_hook_on_define?
+ end
+ end
+ end
+
it "should provide a mechanism for returning set values" do
@settings[:one] = "other"
@settings[:one].should == "other"
-- 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.
