Merge pull request puppetlabs#679 from jeffweiss/ticket/master/13898_…

…fail_face_on_option_clash

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

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

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

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
 

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

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
 

lib/puppet/face/module/install.rb

@@ -108,26 +108,6 @@
       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."
       description <<-EOT
@@ -136,25 +116,8 @@
       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

lib/puppet/face/module/list.rb

@@ -13,23 +13,6 @@
     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 "--tree" do
       summary "Whether to show dependencies as a tree view"
     end
@@ -85,7 +68,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 => {

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
 

lib/puppet/face/module/uninstall.rb

@@ -40,14 +40,6 @@
       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"
       description <<-EOT
@@ -56,17 +48,10 @@
       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

lib/puppet/face/module/upgrade.rb

@@ -46,14 +46,6 @@
       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."
       description <<-EOT
@@ -64,6 +56,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
 

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

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

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

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

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
 

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)

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