Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

(#13898) Fail Face when option collides w/ setting

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 to
Puppet::Util::Settings and deprecate usage of StringSetting.setbycli
because StringSetting provides metadata for the definition of setting,
whereas Setting 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.
  • Loading branch information...
commit d62b3c109b17dad3031a8e47c061b101b8f69c1b 1 parent 32172d5
@jeffweiss jeffweiss authored
View
2  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
View
12 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
View
1  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
View
1  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
View
1  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
View
65 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
View
34 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 => {
View
1  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
View
30 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
View
15 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
View
22 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
View
14 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
View
12 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
View
15 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
View
17 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
View
4 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)
View
2  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
View
8 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
View
10 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
View
8 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
View
67 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
View
45 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
Please sign in to comment.
Something went wrong with that request. Please try again.