From 0b97bd0df57dd6732db5f52fc7360f61e74dba2e Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 5 Apr 2011 11:36:18 -0700 Subject: (#6972) Clean up OptParse name extraction a little. During testing, an obvious cleanup showed up for the name extraction here, so we implement it. This extends the regexp to better extract the data we want rather than hacking it up post-match and having to do extra validation to make sure it actually worked. Reviewed-By: Dan Bode --- lib/puppet/string/option.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 -- cgit From 7e7d246bf46349c904c76a31951d4a40c200790b Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 5 Apr 2011 11:37:51 -0700 Subject: (#6972) Recognize puppet global options in pre-parse. This extends the CLI pre-parse phase to identify both string *and* global options out of the Puppet settings/defaults system. This makes the regular CLI support for setting Puppet configuration globals work as expected. This moves us along the line of supporting these options more fully. Reviewed-By: Dan Bode --- lib/puppet/application/string_base.rb | 22 +++++++++++++++++++++- spec/unit/application/string_base_spec.rb | 29 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) 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/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 -- cgit From a03790d82a2c190d6f00ee7677617a7be04aa85d Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 5 Apr 2011 18:39:19 -0700 Subject: (#6972) Handle ca-location in the certificate string. This ports the existing certificate location configuration to be a string option, and then uses that to change the configuration. This will leak state between calls, which is somewhat unavoidable, but should at least get the basic stuff right for the CLI. We eventually need the CA string to be supported by a stateless internal CA implementation that allows us to do the right thing overall. Reviewed-By: Dan Bode --- lib/puppet/application/certificate.rb | 15 +++------------ lib/puppet/string/certificate.rb | 17 +++++++++++++++++ spec/unit/application/certificate_spec.rb | 16 ++++++++++------ spec/unit/string/certificate_spec.rb | 14 +++++++++++--- 4 files changed, 41 insertions(+), 21 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/string/certificate.rb b/lib/puppet/string/certificate.rb index b231cafb1..fdb0bc9f4 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,6 +37,7 @@ 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) 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/string/certificate_spec.rb b/spec/unit/string/certificate_spec.rb index f6d53688b..4afd581bf 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 "The string itself is broken in this release." + Puppet::SSL::Host.expects(:ca_location=).with(:foo) + Puppet::SSL::Host.indirection.expects(:search) + subject.list :ca_location => :foo + end end -- cgit From 27bd1adb7cc43bfdeb8fb941418cfce3a7f694ef Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 5 Apr 2011 19:08:54 -0700 Subject: (#6983) mark test pending until string is fixed... The certificate string is broken, and won't allow you to either search or save certificates. Given that, mark the test on it broken until that is completed. Reviewed-By: Dan Bode --- lib/puppet/string/certificate.rb | 1 - spec/unit/string/certificate_spec.rb | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/puppet/string/certificate.rb b/lib/puppet/string/certificate.rb index fdb0bc9f4..e8773ae2e 100644 --- a/lib/puppet/string/certificate.rb +++ b/lib/puppet/string/certificate.rb @@ -43,5 +43,4 @@ Puppet::String::Indirector.define(:certificate, '0.0.1') do Puppet::SSL::Host.indirection.save(host) end end - end diff --git a/spec/unit/string/certificate_spec.rb b/spec/unit/string/certificate_spec.rb index 4afd581bf..9fdc5aab8 100755 --- a/spec/unit/string/certificate_spec.rb +++ b/spec/unit/string/certificate_spec.rb @@ -6,9 +6,9 @@ describe Puppet::String[:certificate, '0.0.1'] do end it "should set the ca location when invoked" do - pending "The string itself is broken in this release." + pending "#6983: This is broken in the actual string..." Puppet::SSL::Host.expects(:ca_location=).with(:foo) - Puppet::SSL::Host.indirection.expects(:search) - subject.list :ca_location => :foo + Puppet::SSL::Host.indirection.expects(:save) + subject.sign :ca_location => :foo end end -- cgit