[Pkg-puppet-devel] [SCM] Puppet packaging for Debian branch, experimental, updated. debian/2.6.8-1-844-g7ec39d5
Daniel Pittman
daniel at puppetlabs.com
Tue May 10 08:13:28 UTC 2011
The following commit has been merged in the experimental branch:
commit 7b4d9367b391f75983868046d30928ebc8411f50
Author: Daniel Pittman <daniel at puppetlabs.com>
Date: Tue Apr 12 11:45:05 2011 -0700
(#6962) Move option handling into #parse_options, not #preinit.
Logically, the extra work around option parsing for faces belongs in the
application parse_options method, not hidden in the step before. This commit
moves that to the right place and fixes the fallout from that strange early
design decision.
Along the way we unify error reporting for invalid options so that all the
code paths result in the same externally detected failures.
Reviewed-By: Matt Robinson <matt at puppetlabs.com>
diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index 4c5a5a9..f3a7497 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -359,14 +359,10 @@ class Application
end
end
- # scan command line.
- begin
- option_parser.parse!(self.command_line.args)
- rescue OptionParser::ParseError => detail
- $stderr.puts detail
- $stderr.puts "Try 'puppet #{command_line.subcommand_name} --help'"
- exit(1)
- end
+ # Scan command line. We just hand any exceptions to our upper levels,
+ # rather than printing help and exiting, so that we can meaningfully
+ # respond with context-sensitive help if we want to. --daniel 2011-04-12
+ option_parser.parse!(self.command_line.args)
end
def handlearg(opt, arg)
diff --git a/lib/puppet/application/faces_base.rb b/lib/puppet/application/faces_base.rb
index 288b500..f1b77f2 100644
--- a/lib/puppet/application/faces_base.rb
+++ b/lib/puppet/application/faces_base.rb
@@ -1,5 +1,6 @@
require 'puppet/application'
require 'puppet/faces'
+require 'optparse'
class Puppet::Application::FacesBase < Puppet::Application
should_parse_config
@@ -50,11 +51,13 @@ class Puppet::Application::FacesBase < Puppet::Application
$stderr.puts "Cancelling Face"
exit(0)
end
+ end
+ def parse_options
# We need to parse enough of the command line out early, to identify what
# the action is, so that we can obtain the full set of options to parse.
- # TODO: These should be configurable versions, through a global
+ # REVISIT: These should be configurable versions, through a global
# '--version' option, but we don't implement that yet... --daniel 2011-03-29
@type = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
@face = Puppet::Faces[@type, :current]
@@ -88,25 +91,30 @@ class Puppet::Application::FacesBase < Puppet::Application
index += 1 # ...so skip the argument.
end
else
- raise ArgumentError, "Unknown option #{item.sub(/=.*$/, '').inspect}"
+ raise OptionParser::InvalidOption.new(item.sub(/=.*$/, ''))
end
else
action = @face.get_action(item.to_sym)
if action.nil? then
- raise ArgumentError, "#{@face} does not have an #{item.inspect} action!"
+ raise OptionParser::InvalidArgument.new("#{@face} does not have an #{item} action")
end
@action = action
end
end
- @action or raise ArgumentError, "No action given on the command line!"
+ unless @action
+ raise OptionParser::MissingArgument.new("No action given on the command line")
+ end
- # Finally, we can interact with the default option code to build behaviour
+ # Now we can interact with the default option code to build behaviour
# around the full set of options we now know we support.
@action.options.each do |option|
option = @action.get_option(option) # make it the object.
self.class.option(*option.optparse) # ...and make the CLI parse it.
end
+
+ # ...and invoke our parent to parse all the command line options.
+ super
end
def find_global_settings_argument(item)
diff --git a/spec/unit/application/certificate_spec.rb b/spec/unit/application/certificate_spec.rb
index 6153d95..27d6ac8 100755
--- a/spec/unit/application/certificate_spec.rb
+++ b/spec/unit/application/certificate_spec.rb
@@ -6,12 +6,15 @@ describe Puppet::Application::Certificate do
# so is this actually a valuable test? --daniel 2011-04-07
subject.command_line.stubs(:args).returns %w{list}
subject.preinit
+ subject.parse_options
subject.should respond_to(:handle_ca_location)
end
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.preinit
+ subject.parse_options
+ subject.setup
subject.arguments.should == [{ :ca_location => "local" }]
end
end
diff --git a/spec/unit/application/faces_base_spec.rb b/spec/unit/application/faces_base_spec.rb
index a6df55a..18bd302 100755
--- a/spec/unit/application/faces_base_spec.rb
+++ b/spec/unit/application/faces_base_spec.rb
@@ -23,9 +23,7 @@ describe Puppet::Application::FacesBase do
let :app do
app = Puppet::Application::FacesBase::Basetest.new
- app.stubs(:exit)
- app.stubs(:puts)
- app.command_line.stubs(:subcommand_name).returns 'subcommand'
+ app.command_line.stubs(:subcommand_name).returns('subcommand')
Puppet::Util::Log.stubs(:newdestination)
app
end
@@ -39,7 +37,7 @@ describe Puppet::Application::FacesBase do
end
end
- describe "#preinit" do
+ describe "#parse_options" do
before :each do
app.command_line.stubs(:args).returns %w{}
end
@@ -56,6 +54,7 @@ describe Puppet::Application::FacesBase do
Signal.stubs(:trap)
app.command_line.stubs(:args).returns %w{foo}
app.preinit
+ app.parse_options
end
it "should set the faces based on the type" do
@@ -73,54 +72,54 @@ describe Puppet::Application::FacesBase do
end
it "should fail if no action is given" do
- expect { app.preinit }.
- should raise_error ArgumentError, /No action given/
+ expect { app.preinit; app.parse_options }.
+ to raise_error OptionParser::MissingArgument, /No action given/
end
it "should report a sensible error when options with = fail" do
app.command_line.stubs(:args).returns %w{--action=bar foo}
- expect { app.preinit }.
- should raise_error ArgumentError, /Unknown option "--action"/
+ expect { app.preinit; app.parse_options }.
+ to raise_error OptionParser::InvalidOption, /invalid option: --action/
end
it "should fail if an action option is before the action" do
app.command_line.stubs(:args).returns %w{--action foo}
- expect { app.preinit }.
- should raise_error ArgumentError, /Unknown option "--action"/
+ expect { app.preinit; app.parse_options }.
+ to raise_error OptionParser::InvalidOption, /invalid option: --action/
end
it "should fail if an unknown option is before the action" do
app.command_line.stubs(:args).returns %w{--bar foo}
- expect { app.preinit }.
- should raise_error ArgumentError, /Unknown option "--bar"/
+ expect { app.preinit; app.parse_options }.
+ to raise_error OptionParser::InvalidOption, /invalid option: --bar/
end
- it "should not fail if an unknown option is after the action" do
+ it "should fail if an unknown option is after the action" do
app.command_line.stubs(:args).returns %w{foo --bar}
- app.preinit
- app.action.name.should == :foo
- app.face.should_not be_option :bar
- app.action.should_not be_option :bar
+ expect { app.preinit; app.parse_options }.
+ to raise_error OptionParser::InvalidOption, /invalid option: --bar/
end
it "should accept --bar as an argument to a mandatory option after action" do
app.command_line.stubs(:args).returns %w{foo --mandatory --bar}
- app.preinit and app.parse_options
+ app.preinit
+ app.parse_options
app.action.name.should == :foo
app.options.should == { :mandatory => "--bar" }
end
it "should accept --bar as an argument to a mandatory option before action" do
app.command_line.stubs(:args).returns %w{--mandatory --bar foo}
- app.preinit and app.parse_options
+ app.preinit
+ app.parse_options
app.action.name.should == :foo
app.options.should == { :mandatory => "--bar" }
end
it "should not skip when --foo=bar is given" do
app.command_line.stubs(:args).returns %w{--mandatory=bar --bar foo}
- expect { app.preinit }.
- should raise_error ArgumentError, /Unknown option "--bar"/
+ expect { app.preinit; app.parse_options }.
+ to raise_error OptionParser::InvalidOption, /invalid option: --bar/
end
{ "boolean options before" => %w{--trace foo},
@@ -128,7 +127,8 @@ describe Puppet::Application::FacesBase do
}.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
+ app.preinit
+ app.parse_options
Puppet[:trace].should be_true
end
end
@@ -138,7 +138,8 @@ describe Puppet::Application::FacesBase do
}.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
+ app.preinit
+ app.parse_options
Puppet[:syslogfacility].should == "user1"
end
end
@@ -148,19 +149,25 @@ describe Puppet::Application::FacesBase do
describe "#setup" do
it "should remove the action name from the arguments" do
app.command_line.stubs(:args).returns %w{--mandatory --bar foo}
- app.preinit and app.parse_options and app.setup
+ app.preinit
+ app.parse_options
+ app.setup
app.arguments.should == [{ :mandatory => "--bar" }]
end
it "should pass positional arguments" do
app.command_line.stubs(:args).returns %w{--mandatory --bar foo bar baz quux}
- app.preinit and app.parse_options and app.setup
+ app.preinit
+ app.parse_options
+ app.setup
app.arguments.should == ['bar', 'baz', 'quux', { :mandatory => "--bar" }]
end
end
describe "#main" do
- before do
+ before :each do
+ app.expects(:exit).with(0)
+
app.face = Puppet::Faces[:basetest, '0.0.1']
app.action = app.face.get_action(:foo)
app.format = :pson
@@ -174,6 +181,7 @@ describe Puppet::Application::FacesBase do
it "should use its render method to render any result" do
app.expects(:render).with(app.arguments.length + 1)
+ app.stubs(:puts) # meh. Don't print nil, thanks. --daniel 2011-04-12
app.main
end
end
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 740b76f..a1a46c8 100755
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -357,16 +357,6 @@ describe Puppet::Application do
end
end
-
- it "should exit if OptionParser raises an error" do
- $stderr.stubs(:puts)
- OptionParser.any_instance.stubs(:parse!).raises(OptionParser::ParseError.new("blah blah"))
-
- @app.expects(:exit)
-
- lambda { @app.parse_options }.should_not raise_error
- end
-
end
describe "when calling default setup" do
--
Puppet packaging for Debian
More information about the Pkg-puppet-devel
mailing list