Please review pull request #679: (#13898) Fail Face when option collides w/ setting opened by (jeffweiss)
Description:
Change Puppet::Interface::Option to prohibit options on Faces that have
the same name as an existing Puppet setting.
Move functionality of Puppet::Util::Setting::StringSetting.setbycli toPuppet::Util::Settings and deprecate usage of StringSetting.setbycli
because StringSetting provides metadata for the definition of setting,
whereas Settings contains both the value and the origin of the value,
whether :cli, :memory, :application_defaults, etc.
Change ca application and certificate Face to properly handle all
permutations of --dns_alt_names (Puppet setting) and --dns-alt-names
(Face option).
Remove "documentation only" options of :modulepath and :environment from
module Face.
Prior to this commit, Faces could declare options that collided with
existing Puppet settings, which has caused unexpected behavior for the
Face. This commit explicitly prohibits the Face from declaring an
option which is already a Puppet setting. This is a potentially
breaking change for externally developed Faces.
- Opened: Wed Apr 18 05:51:17 UTC 2012
- Based on: puppetlabs:master (7e22550553515cc90ae94c5e7c847f15c0313119)
- Requested merge: jeffweiss:ticket/master/13898_fail_face_on_option_clash (d62b3c109b17dad3031a8e47c061b101b8f69c1b)
Diff follows:
diff --git a/lib/puppet/application/cert.rb b/lib/puppet/application/cert.rb
index 0cd749f..f8ece5f 100644
--- a/lib/puppet/application/cert.rb
+++ b/lib/puppet/application/cert.rb
@@ -209,7 +209,7 @@ def setup
# the data. This will do the right thing for non-local certificates, in
# that the command line but *NOT* the config file option will apply.
if subcommand == :generate
- if Puppet.settings.setting(:dns_alt_names).setbycli
+ if Puppet.settings.set_by_cli?(:dns_alt_names)
options[:dns_alt_names] = Puppet[:dns_alt_names]
end
end
diff --git a/lib/puppet/face/certificate.rb b/lib/puppet/face/certificate.rb
index e0e0fb5..80d7583 100644
--- a/lib/puppet/face/certificate.rb
+++ b/lib/puppet/face/certificate.rb
@@ -60,6 +60,18 @@
when_invoked do |name, options|
host = Puppet::SSL::Host.new(name)
+
+ # We have a weird case where we have --dns_alt_names from Puppet, but
+ # this option is --dns-alt-names. Until we can get rid of --dns-alt-names
+ # or do a global tr('-', '_'), we have to support both.
+ # In supporting both, we'll use Puppet[:dns_alt_names] if specified on
+ # command line. We'll use options[:dns_alt_names] if specified on
+ # command line. If both specified, we'll fail.
+ # jeffweiss 17 april 2012
+
+ global_setting_from_cli = Puppet.settings.set_by_cli?(:dns_alt_names) == true
+ raise ArgumentError, "Can't specify both --dns_alt_names and --dns-alt-names" if options[:dns_alt_names] and global_setting_from_cli
+ options[:dns_alt_names] = Puppet[:dns_alt_names] if global_setting_from_cli
# If dns_alt_names are specified via the command line, we will always add
# them. Otherwise, they will default to the config file setting iff this
diff --git a/lib/puppet/face/module/build.rb b/lib/puppet/face/module/build.rb
index eec482e..fec13d8 100644
--- a/lib/puppet/face/module/build.rb
+++ b/lib/puppet/face/module/build.rb
@@ -26,6 +26,7 @@
arguments "<path>"
when_invoked do |path, options|
+ Puppet::Module::Tool.set_option_defaults options
Puppet::Module::Tool::Applications::Builder.run(path, options)
end
diff --git a/lib/puppet/face/module/changes.rb b/lib/puppet/face/module/changes.rb
index 602e423..b58c0a7 100644
--- a/lib/puppet/face/module/changes.rb
+++ b/lib/puppet/face/module/changes.rb
@@ -20,6 +20,7 @@
arguments "<path>"
when_invoked do |path, options|
+ Puppet::Module::Tool.set_option_defaults options
root_path = Puppet::Module::Tool.find_module_root(path)
Puppet::Module::Tool::Applications::Checksummer.run(root_path, options)
end
diff --git a/lib/puppet/face/module/generate.rb b/lib/puppet/face/module/generate.rb
index 8f1622c..1b332f6 100644
--- a/lib/puppet/face/module/generate.rb
+++ b/lib/puppet/face/module/generate.rb
@@ -32,6 +32,7 @@
arguments "<name>"
when_invoked do |name, options|
+ Puppet::Module::Tool.set_option_defaults options
Puppet::Module::Tool::Applications::Generator.run(name, options)
end
diff --git a/lib/puppet/face/module/install.rb b/lib/puppet/face/module/install.rb
index c3e0754..54489b1 100644
--- a/lib/puppet/face/module/install.rb
+++ b/lib/puppet/face/module/install.rb
@@ -108,25 +108,25 @@
EOT
end
- option "--modulepath MODULEPATH" do
- default_to { Puppet.settings[:modulepath] }
- summary "Which directories to look for modules in"
- description <<-EOT
- The list of directories to check for modules. When installing a new
- module, this setting determines where the module tool will look for
- its dependencies. If the `--target dir` option is not specified, the
- first directory in the modulepath will also be used as the install
- directory.
-
- When installing a module into an environment whose modulepath is
- specified in puppet.conf, you can use the `--environment` option
- instead, and its modulepath will be used automatically.
-
- This setting should be a list of directories separated by the path
- separator character. (The path separator is `:` on Unix-like platforms
- and `;` on Windows.)
- EOT
- end
+# option "--modulepath MODULEPATH" do
+# default_to { Puppet.settings[:modulepath] }
+# summary "Which directories to look for modules in"
+# description <<-EOT
+# The list of directories to check for modules. When installing a new
+# module, this setting determines where the module tool will look for
+# its dependencies. If the `--target dir` option is not specified, the
+# first directory in the modulepath will also be used as the install
+# directory.
+#
+# When installing a module into an environment whose modulepath is
+# specified in puppet.conf, you can use the `--environment` option
+# instead, and its modulepath will be used automatically.
+#
+# This setting should be a list of directories separated by the path
+# separator character. (The path separator is `:` on Unix-like platforms
+# and `;` on Windows.)
+# EOT
+# end
option "--version VER", "-v VER" do
summary "Module version to install."
@@ -136,25 +136,18 @@
EOT
end
- option "--environment NAME" do
- default_to { "production" }
- summary "The target environment to install modules into."
- description <<-EOT
- The target environment to install modules into. Only applicable if
- multiple environments (with different modulepaths) have been
- configured in puppet.conf.
- EOT
- end
+# option "--environment NAME" do
+# default_to { "production" }
+# summary "The target environment to install modules into."
+# description <<-EOT
+# The target environment to install modules into. Only applicable if
+# multiple environments (with different modulepaths) have been
+# configured in puppet.conf.
+# EOT
+# end
when_invoked do |name, options|
- sep = File::PATH_SEPARATOR
- if options[:target_dir]
- options[:modulepath] = "#{options[:target_dir]}#{sep}#{options[:modulepath]}"
- end
-
- Puppet.settings[:modulepath] = options[:modulepath]
- options[:target_dir] = Puppet.settings[:modulepath].split(sep).first
-
+ Puppet::Module::Tool.set_option_defaults options
Puppet.notice "Preparing to install into #{options[:target_dir]} ..."
Puppet::Module::Tool::Applications::Installer.run(name, options)
end
diff --git a/lib/puppet/face/module/list.rb b/lib/puppet/face/module/list.rb
index e3acbd9..5ff6fa1 100644
--- a/lib/puppet/face/module/list.rb
+++ b/lib/puppet/face/module/list.rb
@@ -13,22 +13,22 @@
HEREDOC
returns "hash of paths to module objects"
- option "--environment NAME" do
- default_to { "production" }
- summary "Which environments' modules to list"
- description <<-EOT
- Which environments' modules to list.
- EOT
- end
-
- option "--modulepath MODULEPATH" do
- summary "Which directories to look for modules in"
- description <<-EOT
- Which directories to look for modules in; use the system path separator
- character (`:` on Unix-like systems and `;` on Windows) to specify
- multiple directories.
- EOT
- end
+# option "--environment NAME" do
+# default_to { "production" }
+# summary "Which environments' modules to list"
+# description <<-EOT
+# Which environments' modules to list.
+# EOT
+# end
+
+# option "--modulepath MODULEPATH" do
+# summary "Which directories to look for modules in"
+# description <<-EOT
+# Which directories to look for modules in; use the system path separator
+# character (`:` on Unix-like systems and `;` on Windows) to specify
+# multiple directories.
+# EOT
+# end
option "--tree" do
summary "Whether to show dependencies as a tree view"
@@ -85,7 +85,7 @@
output = ''
Puppet[:modulepath] = options[:modulepath] if options[:modulepath]
- environment = Puppet::Node::Environment.new(options[:production])
+ environment = Puppet::Node::Environment.new(options[:environment])
error_types = {
:non_semantic_version => {
diff --git a/lib/puppet/face/module/search.rb b/lib/puppet/face/module/search.rb
index 169f640..c0fd465 100644
--- a/lib/puppet/face/module/search.rb
+++ b/lib/puppet/face/module/search.rb
@@ -21,6 +21,7 @@
arguments "<search_term>"
when_invoked do |term, options|
+ Puppet::Module::Tool.set_option_defaults options
Puppet::Module::Tool::Applications::Searcher.run(term, options)
end
diff --git a/lib/puppet/face/module/uninstall.rb b/lib/puppet/face/module/uninstall.rb
index 1effa6c..886f3a2 100644
--- a/lib/puppet/face/module/uninstall.rb
+++ b/lib/puppet/face/module/uninstall.rb
@@ -40,13 +40,13 @@
EOT
end
- option "--environment NAME" do
- default_to { "production" }
- summary "The target environment to uninstall modules from."
- description <<-EOT
- The target environment to uninstall modules from.
- EOT
- end
+# option "--environment NAME" do
+# default_to { "production" }
+# summary "The target environment to uninstall modules from."
+# description <<-EOT
+# The target environment to uninstall modules from.
+# EOT
+# end
option "--version=" do
summary "The version of the module to uninstall"
@@ -56,17 +56,17 @@
EOT
end
- option "--modulepath=" do
- summary "The target directory to search for modules."
- description <<-EOT
- The target directory to search for modules.
- EOT
- end
+# option "--modulepath=" do
+# summary "The target directory to search for modules."
+# description <<-EOT
+# The target directory to search for modules.
+# EOT
+# end
when_invoked do |name, options|
- Puppet[:modulepath] = options[:modulepath] if options[:modulepath]
name = name.gsub('/', '-')
-
+
+ Puppet::Module::Tool.set_option_defaults options
Puppet.notice "Preparing to uninstall '#{name}'" << (options[:version] ? " (#{colorize(:cyan, options[:version].sub(/^(?=\d)/, 'v'))})" : '') << " ..."
Puppet::Module::Tool::Applications::Uninstaller.run(name, options)
end
diff --git a/lib/puppet/face/module/upgrade.rb b/lib/puppet/face/module/upgrade.rb
index e62a7c5..d77e394 100644
--- a/lib/puppet/face/module/upgrade.rb
+++ b/lib/puppet/face/module/upgrade.rb
@@ -46,13 +46,13 @@
EOT
end
- option "--environment NAME" do
- default_to { "production" }
- summary "The target environment to search for modules."
- description <<-EOT
- The target environment to search for modules.
- EOT
- end
+# option "--environment NAME" do
+# default_to { "production" }
+# summary "The target environment to search for modules."
+# description <<-EOT
+# The target environment to search for modules.
+# EOT
+# end
option "--version=" do
summary "The version of the module to upgrade to."
@@ -64,6 +64,7 @@
when_invoked do |name, options|
name = name.gsub('/', '-')
Puppet.notice "Preparing to upgrade '#{name}' ..."
+ Puppet::Module::Tool.set_option_defaults options
Puppet::Module::Tool::Applications::Upgrader.new(name, options).run
end
diff --git a/lib/puppet/interface/option.rb b/lib/puppet/interface/option.rb
index 01f6f23..3d0a9c3 100644
--- a/lib/puppet/interface/option.rb
+++ b/lib/puppet/interface/option.rb
@@ -18,7 +18,18 @@ def initialize(parent, *declaration, &block)
@optparse << item
# Duplicate checking...
- name = optparse_to_name(item)
+ # for our duplicate checking purpose, we don't make a check with the
+ # translated '-' -> '_'. Right now, we do that on purpose because of
+ # a duplicated option made publicly available on certificate and ca
+ # faces for dns alt names. Puppet defines 'dns_alt_names', those
+ # faces include 'dns-alt-names'. We can't get rid of 'dns-alt-names'
+ # yet, so we need to do our duplicate checking on the untranslated
+ # version of the option.
+ # jeffweiss 17 april 2012
+ name = optparse_to_optionname(item)
+ if Puppet.settings.include? name then
+ raise ArgumentError, "#{item.inspect}: already defined in puppet"
+ end
if dup = dups[name] then
raise ArgumentError, "#{item.inspect}: duplicates existing alias #{dup.inspect} in #{@parent}"
else
@@ -62,11 +73,16 @@ def to_s
@name.to_s.tr('_', '-')
end
- def optparse_to_name(declaration)
+ def optparse_to_optionname(declaration)
unless found = declaration.match(/^-+(?:\[no-\])?([^ =]+)/) then
raise ArgumentError, "Can't find a name in the declaration #{declaration.inspect}"
end
- name = found.captures.first.tr('-', '_')
+ name = found.captures.first
+ end
+
+
+ def optparse_to_name(declaration)
+ name = optparse_to_optionname(declaration).tr('-', '_')
raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
name.to_sym
end
diff --git a/lib/puppet/module_tool.rb b/lib/puppet/module_tool.rb
index a05ac8e..f0556a4 100644
--- a/lib/puppet/module_tool.rb
+++ b/lib/puppet/module_tool.rb
@@ -84,6 +84,20 @@ def self.build_tree(mods, dir)
build_tree(mod[:dependencies], dir)
end
end
+
+ def self.set_option_defaults(options)
+ sep = File::PATH_SEPARATOR
+
+ prepend_target_dir = !! options[:target_dir]
+
+ options[:modulepath] ||= Puppet.settings[:modulepath]
+ options[:environment] ||= Puppet.settings[:environment]
+ options[:modulepath] = "#{options[:target_dir]}#{sep}#{options[:modulepath]}" if prepend_target_dir
+ Puppet[:modulepath] = options[:modulepath]
+ Puppet[:environment] = options[:environment]
+
+ options[:target_dir] = options[:modulepath].split(sep).first
+ end
end
end
end
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index 30c5303..d6b54a3 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -622,6 +622,14 @@ def legacy_to_mode(type, param)
end
type
end
+
+ # Allow later inspection to determine if the setting was set on the
+ # command line, or through some other code path. Used for the
+ # `dns_alt_names` option during cert generate. --daniel 2011-10-18
+ def set_by_cli?(param)
+ param = param.to_sym
+ !@values[:cli][param].nil?
+ end
def set_value(param, value, type, options = {})
param = param.to_sym
@@ -643,10 +651,6 @@ def set_value(param, value, type, options = {})
end
type = legacy_to_mode(type, param)
@sync.synchronize do # yay, thread-safe
- # Allow later inspection to determine if the setting was set on the
- # command line, or through some other code path. Used for the
- # `dns_alt_names` option during cert generate. --daniel 2011-10-18
- setting.setbycli = true if type == :cli
@values[type][param] = value
@cache.clear
diff --git a/lib/puppet/util/settings/string_setting.rb b/lib/puppet/util/settings/string_setting.rb
index 0a21871..7405952 100644
--- a/lib/puppet/util/settings/string_setting.rb
+++ b/lib/puppet/util/settings/string_setting.rb
@@ -1,11 +1,24 @@
# The base element type.
class Puppet::Util::Settings::StringSetting
- attr_accessor :name, :section, :default, :setbycli, :call_on_define
+ attr_accessor :name, :section, :default, :call_on_define
attr_reader :desc, :short
def desc=(value)
@desc = value.gsub(/^\s*/, '')
end
+
+ #added as a proper method, only to generate a deprecation warning
+ #and return value from
+ def setbycli
+ Puppet.deprecation_warning "Puppet.settings.setting(#{name}).setbycli is deprecated. Use Puppet.settings.set_by_cli?(#{name}) instead."
+ @settings.set_by_cli?(name)
+ end
+
+ def setbycli=(value)
+ Puppet.deprecation_warning "Puppet.settings.setting(#{name}).setbycli= is deprecated. You should not manually set that values were specified on the command line."
+ @settings.set_value(name, @settings[name], :cli) if value
+ raise ArgumentError, "Cannot unset setbycli" unless value
+ end
# get the arguments in getopt format
def getopt_args
diff --git a/spec/unit/face/certificate_spec.rb b/spec/unit/face/certificate_spec.rb
index 8d0cbcf..e8c0d3c 100755
--- a/spec/unit/face/certificate_spec.rb
+++ b/spec/unit/face/certificate_spec.rb
@@ -127,6 +127,23 @@
csr.subject_alt_names.should =~ expected
end
+
+ it "should use the global setting if set by CLI" do
+ Puppet.settings.set_value(:dns_alt_names, 'from,the,cli', :cli)
+
+ subject.generate(hostname, options)
+
+ expected = %W[DNS:from DNS:the DNS:cli DNS:#{hostname}]
+
+ csr.subject_alt_names.should =~ expected
+ end
+
+ it "should generate an error if both set on CLI" do
+ Puppet.settings.set_value(:dns_alt_names, 'from,the,cli', :cli)
+ expect do
+ subject.generate(hostname, options.merge(:dns_alt_names => 'explicit,alt,names'))
+ end.to raise_error ArgumentError, /Can't specify both/
+ end
end
end
diff --git a/spec/unit/face/module/install_spec.rb b/spec/unit/face/module/install_spec.rb
index 9f67800..e685af8 100644
--- a/spec/unit/face/module/install_spec.rb
+++ b/spec/unit/face/module/install_spec.rb
@@ -72,7 +72,7 @@
end
describe "when modulepath option is passed" do
- let(:expected_options) { { :modulepath => fakemodpath, :environment => 'production' } }
+ let(:expected_options) { { :modulepath => fakemodpath, :environment => Puppet[:environment] } }
let(:options) { { :modulepath => fakemodpath } }
describe "when target-dir option is not passed" do
@@ -94,7 +94,7 @@
options[:target_dir] = fakedirpath
expected_options[:target_dir] = fakedirpath
expected_options[:modulepath] = "#{fakedirpath}#{sep}#{fakemodpath}"
-
+
Puppet::Module::Tool::Applications::Installer.
expects(:run).
with("puppetlabs-apache", expected_options)
diff --git a/spec/unit/face/module/search_spec.rb b/spec/unit/face/module/search_spec.rb
index 51f62bd..ca6dcf8 100644
--- a/spec/unit/face/module/search_spec.rb
+++ b/spec/unit/face/module/search_spec.rb
@@ -141,7 +141,7 @@
it "should accept the --module-repository option" do
options[:module_repository] = "http://forge.example.com"
- Puppet::Module::Tool::Applications::Searcher.expects(:run).with("puppetlabs-apache", options).once
+ Puppet::Module::Tool::Applications::Searcher.expects(:run).with("puppetlabs-apache", has_entries(options)).once
subject.search("puppetlabs-apache", options)
end
end
diff --git a/spec/unit/face/module/uninstall_spec.rb b/spec/unit/face/module/uninstall_spec.rb
index a157df5..2343876 100644
--- a/spec/unit/face/module/uninstall_spec.rb
+++ b/spec/unit/face/module/uninstall_spec.rb
@@ -25,7 +25,7 @@
it "should accept the --environment option" do
options[:environment] = "development"
expected_options = { :environment => 'development' }
- Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", expected_options).once
+ Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", has_entries(expected_options)).once
subject.uninstall("puppetlabs-apache", options)
end
@@ -35,7 +35,7 @@
:modulepath => '/foo/puppet/modules',
:environment => 'production',
}
- Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", expected_options).once
+ Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", has_entries(expected_options)).once
subject.uninstall("puppetlabs-apache", options)
end
@@ -45,7 +45,7 @@
:version => '1.0.0',
:environment => 'production',
}
- Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", expected_options).once
+ Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", has_entries(expected_options)).once
subject.uninstall("puppetlabs-apache", options)
end
@@ -55,7 +55,7 @@
:environment => 'production',
:force => true,
}
- Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", expected_options).once
+ Puppet::Module::Tool::Applications::Uninstaller.expects(:run).with("puppetlabs-apache", has_entries(expected_options)).once
subject.uninstall("puppetlabs-apache", options)
end
end
diff --git a/spec/unit/interface/option_builder_spec.rb b/spec/unit/interface/option_builder_spec.rb
index 3e91c68..1b95fe7 100755
--- a/spec/unit/interface/option_builder_spec.rb
+++ b/spec/unit/interface/option_builder_spec.rb
@@ -8,6 +8,14 @@
should be_an_instance_of Puppet::Interface::Option
end
+ Puppet.settings.each do |name, value|
+ it "should fail when option #{name.inspect} already exists in puppet core" do
+ expect do
+ Puppet::Interface::OptionBuilder.build(face, "--#{name}")
+ end.should raise_error ArgumentError, /already defined/
+ end
+ end
+
it "should work with an empty block" do
option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
# This block deliberately left blank.
@@ -72,6 +80,6 @@
end
opt.should_not be_required
end
-
+
end
end
diff --git a/spec/unit/interface/option_spec.rb b/spec/unit/interface/option_spec.rb
index e73561f..106643f 100755
--- a/spec/unit/interface/option_spec.rb
+++ b/spec/unit/interface/option_spec.rb
@@ -59,6 +59,14 @@
Puppet::Interface::Option.new(face, "--foo").
should be_instance_of Puppet::Interface::Option
end
+
+ Puppet.settings.each do |name, value|
+ it "should fail when option #{name.inspect} already exists in puppet core" do
+ expect do
+ Puppet::Interface::Option.new(face, "--#{name}")
+ end.should raise_error ArgumentError, /already defined/
+ end
+ end
describe "#to_s" do
it "should transform a symbol into a string" do
diff --git a/spec/unit/module_tool_spec.rb b/spec/unit/module_tool_spec.rb
index 1517f30..4b68cb5 100644
--- a/spec/unit/module_tool_spec.rb
+++ b/spec/unit/module_tool_spec.rb
@@ -110,4 +110,71 @@
TREE
end
end
+ describe '.set_option_defaults' do
+ [:environment, :modulepath].each do |value|
+ describe "if #{value} is part of options" do
+ let (:options) { {} }
+ before(:each) do
+ options[value] = "foo"
+ Puppet[value] = "bar"
+ end
+ it "should set Puppet[#{value}] to the options[#{value}]" do
+ subject.set_option_defaults options
+ Puppet[value].should == options[value]
+ end
+ it "should not override options[#{value}]" do
+ subject.set_option_defaults options
+ options[value].should == "foo"
+ end
+ end
+ describe "if #{value} is not part of options" do
+ let (:options) { {} }
+ before(:each) do
+ Puppet[value] = "bar"
+ end
+ it "should populate options[#{value}] with the value of Puppet[#{value}]" do
+ subject.set_option_defaults options
+ Puppet[value].should == options[value]
+ end
+ it "should not override Puppet[#{value}]" do
+ subject.set_option_defaults options
+ Puppet[value].should == "bar"
+ end
+ end
+ end
+
+ describe ':target_dir' do
+ let (:sep) { File::PATH_SEPARATOR }
+ let (:my_fake_path) { "/my/fake/dir#{sep}/my/other/dir"}
+ let (:options) { {:modulepath => my_fake_path}}
+ describe "when not specified" do
+ it "should set options[:target_dir]" do
+ subject.set_option_defaults options
+ options[:target_dir].should_not be_nil
+ end
+ it "should be the first path of options[:modulepath]" do
+ subject.set_option_defaults options
+ options[:target_dir].should == my_fake_path.split(sep).first
+ end
+ end
+ describe "when specified" do
+ let (:my_target_dir) { "/foo/bar" }
+ before(:each) do
+ options[:target_dir] = my_target_dir
+ end
+ it "should not be overridden" do
+ subject.set_option_defaults options
+ options[:target_dir].should == my_target_dir
+ end
+ it "should be prepended to options[:modulepath]" do
+ subject.set_option_defaults options
+ options[:modulepath].split(sep).first.should == my_target_dir
+ end
+ it "should leave the remainder of options[:modulepath] untouched" do
+ subject.set_option_defaults options
+ options[:modulepath].split(sep).drop(1).join(sep).should == my_fake_path
+ end
+ end
+ end
+ end
end
diff --git a/spec/unit/util/settings_spec.rb b/spec/unit/util/settings_spec.rb
index 17a6ff7..d8b3af2 100755
--- a/spec/unit/util/settings_spec.rb
+++ b/spec/unit/util/settings_spec.rb
@@ -162,14 +162,38 @@
@settings[:myval].should == ""
end
- it "should flag settings from the CLI" do
- @settings.handlearg("--myval")
- @settings.setting(:myval).setbycli.should be_true
+ it "should flag string settings from the CLI" do
+ @settings.handlearg("--myval", "12")
+ @settings.set_by_cli?(:myval).should be_true
end
- it "should not flag settings memory" do
+ it "should flag bool settings from the CLI" do
+ @settings[:bool] = false
+ @settings.handlearg("--bool")
+ @settings.set_by_cli?(:bool).should be_true
+ end
+
+ it "should not flag settings memory as from CLI" do
@settings[:myval] = "12"
- @settings.setting(:myval).setbycli.should be_false
+ @settings.set_by_cli?(:myval).should be_false
+ end
+
+ describe "setbycli" do
+ it "should generate a deprecation warning" do
+ Puppet.expects(:deprecation_warning)
+ @settings.setting(:myval).setbycli = true
+ end
+ it "should set the value" do
+ @settings[:myval] = "blah"
+ @settings.setting(:myval).setbycli = true
+ @settings.set_by_cli?(:myval).should be_true
+ end
+ it "should raise error if trying to unset value" do
+ @settings.handlearg("--myval", "blah")
+ expect do
+ @settings.setting(:myval).setbycli = nil
+ end.to raise_error ArgumentError, /unset/
+ end
end
it "should clear the cache when setting getopt-specific values" do
@@ -327,6 +351,17 @@
it "should have a run_mode that defaults to user" do
@settings.run_mode.should == :user
end
+ describe "setbycli" do
+ it "should generate a deprecation warning" do
+ @settings.handlearg("--one", "blah")
+ Puppet.expects(:deprecation_warning)
+ @settings.setting(:one).setbycli
+ end
+ it "should be true" do
+ @settings.handlearg("--one", "blah")
+ @settings.setting(:one).setbycli.should be_true
+ end
+ end
end
describe "when choosing which value to return" do
-- 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.
