Merge branch 'bug/master/6972-setting-CA-location-for-cert-string-no-longer-works'

7 files changed, 93 insertions, 25 deletions

diff --git a/lib/puppet/application/certificate.rb b/lib/puppet/application/certificate.rb
index f4b13ffe0..eacb830b2 100644
--- a/lib/puppet/application/certificate.rb
+++ b/lib/puppet/application/certificate.rb
@@ -1,18 +1,10 @@
 require 'puppet/application/indirection_base'
 
 class Puppet::Application::Certificate < Puppet::Application::IndirectionBase
-
-  # Luke used to call this --ca but that's taken by the global boolean --ca.
-  # Since these options map CA terminology to indirector terminology, it's
-  # now called --ca-location.
-  option "--ca-location CA_LOCATION" do |arg|
-    Puppet::SSL::Host.ca_location = arg.to_sym
-  end
-
   def setup
-
-    unless Puppet::SSL::Host.ca_location
-      raise ArgumentError, "You must have a CA location specified; use --ca-location to specify the location (remote, local, only)"
+    unless options[:ca_location]
+      raise ArgumentError, "You must have a CA location specified;\n" +
+        "use --ca-location to specify the location (remote, local, only)"
     end
 
     location = Puppet::SSL::Host.ca_location
@@ -23,5 +15,4 @@ class Puppet::Application::Certificate < Puppet::Application::IndirectionBase
 
     super
   end
-
 end
diff --git a/lib/puppet/application/string_base.rb b/lib/puppet/application/string_base.rb
index 76b0a46fd..09d02c079 100644
--- a/lib/puppet/application/string_base.rb
+++ b/lib/puppet/application/string_base.rb
@@ -68,7 +68,9 @@ class Puppet::Application::StringBase < Puppet::Application
     until @action or (index += 1) >= command_line.args.length do
       item = command_line.args[index]
       if item =~ /^-/ then
-        option = @string.options.find { |a| item =~ /^-+#{a}\b/ }
+        option = @string.options.find do |name|
+          item =~ /^-+#{name.to_s.gsub(/[-_]/, '[-_]')}(?:[ =].*)?$/
+        end
         if option then
           option = @string.get_option(option)
           # If we have an inline argument, just carry on.  We don't need to
@@ -79,6 +81,12 @@ class Puppet::Application::StringBase < Puppet::Application
             index += 1 unless
               (option.optional_argument? and command_line.args[index + 1] =~ /^-/)
           end
+        elsif option = find_global_settings_argument(item) then
+          unless Puppet.settings.boolean? option.name then
+            # As far as I can tell, we treat non-bool options as always having
+            # a mandatory argument. --daniel 2011-04-05
+            index += 1          # ...so skip the argument.
+          end
         else
           raise ArgumentError, "Unknown option #{item.sub(/=.*$/, '').inspect}"
         end
@@ -101,6 +109,18 @@ class Puppet::Application::StringBase < Puppet::Application
     end
   end
 
+  def find_global_settings_argument(item)
+    Puppet.settings.each do |name, object|
+      object.optparse_args.each do |arg|
+        next unless arg =~ /^-/
+        # sadly, we have to emulate some of optparse here...
+        pattern = /^#{arg.sub('[no-]', '').sub(/[ =].*$/, '')}(?:[ =].*)?$/
+        pattern.match item and return object
+      end
+    end
+    return nil                  # nothing found.
+  end
+
   def setup
     Puppet::Util::Log.newdestination :console
 
diff --git a/lib/puppet/string/certificate.rb b/lib/puppet/string/certificate.rb
index b231cafb1..e8773ae2e 100644
--- a/lib/puppet/string/certificate.rb
+++ b/lib/puppet/string/certificate.rb
@@ -2,9 +2,24 @@ require 'puppet/string/indirector'
 require 'puppet/ssl/host'
 
 Puppet::String::Indirector.define(:certificate, '0.0.1') do
+  # REVISIT: This should use a pre-invoke hook to run the common code that
+  # needs to happen before we invoke any action; that would be much nicer than
+  # the "please repeat yourself" stuff found in here right now.
+  #
+  # option "--ca-location LOCATION" do
+  #   type [:whatever, :location, :symbols]
+  #   hook :before do |value|
+  #     Puppet::SSL::Host.ca_location = value
+  #   end
+  # end
+  #
+  # ...but should I pass the arguments as well?
+  # --daniel 2011-04-05
+  option "--ca-location LOCATION"
 
   action :generate do
     when_invoked do |name, options|
+      Puppet::SSL::Host.ca_location = options[:ca_location].to_sym
       host = Puppet::SSL::Host.new(name)
       host.generate_certificate_request
       host.certificate_request.class.indirection.save(host.certificate_request)
@@ -13,6 +28,7 @@ Puppet::String::Indirector.define(:certificate, '0.0.1') do
 
   action :list do
     when_invoked do |options|
+      Puppet::SSL::Host.ca_location = options[:ca_location].to_sym
       Puppet::SSL::Host.indirection.search("*", {
         :for => :certificate_request,
       }).map { |h| h.inspect }
@@ -21,10 +37,10 @@ Puppet::String::Indirector.define(:certificate, '0.0.1') do
 
   action :sign do
     when_invoked do |name, options|
+      Puppet::SSL::Host.ca_location = options[:ca_location].to_sym
       host = Puppet::SSL::Host.new(name)
       host.desired_state = 'signed'
       Puppet::SSL::Host.indirection.save(host)
     end
   end
-
 end
diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index f499e4b95..be7bbb76e 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -70,10 +70,10 @@ class Puppet::String::Option
   end
 
   def optparse_to_name(declaration)
-    unless found = declaration.match(/^-+([^= ]+)/) or found.length != 1 then
+    unless found = declaration.match(/^-+(?:\[no-\])?([^ =]+)/) then
       raise ArgumentError, "Can't find a name in the declaration #{declaration.inspect}"
     end
-    name = found.captures.first.sub('[no-]', '').tr('-', '_')
+    name = found.captures.first.tr('-', '_')
     raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
     name.to_sym
   end
diff --git a/spec/unit/application/certificate_spec.rb b/spec/unit/application/certificate_spec.rb
index 6666f54f7..3d2215ded 100755
--- a/spec/unit/application/certificate_spec.rb
+++ b/spec/unit/application/certificate_spec.rb
@@ -4,13 +4,17 @@ require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
 require 'puppet/application/certificate'
 
 describe Puppet::Application::Certificate do
-  it "should be a subclass of Puppet::Application::IndirectionBase" do
-    Puppet::Application::Certificate.superclass.should equal(
-      Puppet::Application::IndirectionBase
-    )
+  it "should have a 'ca-location' option" do
+    # REVISIT: This is delegated from the string, and we will have a test
+    # there, so is this actually a valuable test?
+    subject.command_line.stubs(:args).returns %w{list}
+    subject.preinit
+    subject.should respond_to(:handle_ca_location)
   end
 
-  it "should have a 'ca' option" do
-    Puppet::Application::Certificate.new.should respond_to(:handle_ca_location)
+  it "should accept the ca-location option" do
+    subject.command_line.stubs(:args).returns %w{--ca-location local list}
+    subject.preinit and subject.parse_options and subject.setup
+    subject.arguments.should == [{ :ca_location => "local" }]
   end
 end
diff --git a/spec/unit/application/string_base_spec.rb b/spec/unit/application/string_base_spec.rb
index cd24b6c49..3f8ae73b6 100755
--- a/spec/unit/application/string_base_spec.rb
+++ b/spec/unit/application/string_base_spec.rb
@@ -42,6 +42,15 @@ describe Puppet::Application::StringBase do
     app
   end
 
+  describe "#find_global_settings_argument" do
+    it "should not match --ca to --ca-location" do
+      option = mock('ca option', :optparse_args => ["--ca"])
+      Puppet.settings.expects(:each).yields(:ca, option)
+
+      app.find_global_settings_argument("--ca-location").should be_nil
+    end
+  end
+
   describe "#preinit" do
     before :each do
       app.command_line.stubs(:args).returns %w{}
@@ -118,6 +127,26 @@ describe Puppet::Application::StringBase do
         expect { app.preinit }.
           should raise_error ArgumentError, /Unknown option "--bar"/
       end
+
+      { "boolean options before" => %w{--trace foo},
+        "boolean options after"  => %w{foo --trace}
+      }.each do |name, args|
+        it "should accept global boolean settings #{name} the action" do
+          app.command_line.stubs(:args).returns args
+          app.preinit && app.parse_options
+          Puppet[:trace].should be_true
+        end
+      end
+
+      { "before" => %w{--syslogfacility user1 foo},
+        " after" => %w{foo --syslogfacility user1}
+      }.each do |name, args|
+        it "should accept global settings with arguments #{name} the action" do
+          app.command_line.stubs(:args).returns args
+          app.preinit && app.parse_options
+          Puppet[:syslogfacility].should == "user1"
+        end
+      end
     end
   end
 
diff --git a/spec/unit/string/certificate_spec.rb b/spec/unit/string/certificate_spec.rb
index f6d53688b..9fdc5aab8 100755
--- a/spec/unit/string/certificate_spec.rb
+++ b/spec/unit/string/certificate_spec.rb
@@ -1,6 +1,14 @@
-#!/usr/bin/env ruby
-
-require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
+require 'puppet/ssl/host'
 
 describe Puppet::String[:certificate, '0.0.1'] do
+  it "should have a ca-location option" do
+    subject.should be_option :ca_location
+  end
+
+  it "should set the ca location when invoked" do
+    pending "#6983: This is broken in the actual string..."
+    Puppet::SSL::Host.expects(:ca_location=).with(:foo)
+    Puppet::SSL::Host.indirection.expects(:save)
+    subject.sign :ca_location => :foo
+  end
 end