Looks pretty good, Deepak.
This is an overall code improvement, especially the
Report#to_exit_status method extraction. (Although, to be perfectly
nitpicky, I would prefer it to be named simply "exit_status".) I'm
concerned that a spec was removed from
spec/unit/application/puppet.rb. I'd like to know what happened to it
before I +1.
As an aside, the original code contains the strange construct
"sync.synchronize { lock { do_something } }", which seems both
unnecessary (synchronize already acquires and releases a lock for you)
and potentially buggy. I think I can come up with a patch to address
this later.
On Tue, Oct 6, 2009 at 8:11 PM, Deepak Giridharagopal
<[email protected]> wrote:
>
> This option only works when --onetime is specified, as it doesn't make
> much sense to worry about exit codes in the context of a long-running
> daemon.
>
> This required a refactoring of the existing --detailed-exitcodes code,
> as "puppetd" wasn't directly creating a transaction object (like
> "puppet" does).
>
> Added Report::to_exit_status, which did what was previously hard-coded
> into the "puppet" executable.
>
> An Agent's "run" method now returns a value (the result of the
> individual client class' "run" method)
>
> The "puppetd" agent's "run" method now returns a transaction report, as
> that seems like the logical thing to return as the result of applying a
> catalog.
>
> Signed-off-by: Deepak Giridharagopal <[email protected]>
> ---
> lib/puppet/agent.rb | 4 ++-
> lib/puppet/application/puppet.rb | 7 ++---
> lib/puppet/application/puppetd.rb | 16 ++++++++++++-
> lib/puppet/configurer.rb | 8 +++++-
> lib/puppet/transaction/report.rb | 11 +++++++++-
> sbin/puppetd | 9 +++++++-
> spec/unit/application/puppet.rb | 41 +++++++++++-------------------------
> spec/unit/application/puppetd.rb | 28 +++++++++++++++++++++++++
> spec/unit/transaction/report.rb | 23 ++++++++++++++++++++
> 9 files changed, 108 insertions(+), 39 deletions(-)
>
> diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb
> index a188df3..7bd9741 100644
> --- a/lib/puppet/agent.rb
> +++ b/lib/puppet/agent.rb
> @@ -48,14 +48,16 @@ class Puppet::Agent
> return
> end
> splay
> + result = nil
> with_client do |client|
> begin
> - sync.synchronize { lock { client.run(*args) } }
> + sync.synchronize { lock { result = client.run(*args) } }
> rescue => detail
> puts detail.backtrace if Puppet[:trace]
> Puppet.err "Could not run %s: %s" % [client_class, detail]
> end
> end
> + result
> end
>
> def stop
> diff --git a/lib/puppet/application/puppet.rb
> b/lib/puppet/application/puppet.rb
> index a87ae48..5ee589b 100644
> --- a/lib/puppet/application/puppet.rb
> +++ b/lib/puppet/application/puppet.rb
> @@ -127,13 +127,12 @@ Puppet::Application.new(:puppet) do
> # And apply it
> transaction = catalog.apply
>
> - status = 0
> if not Puppet[:noop] and options[:detailed_exitcodes] then
> transaction.generate_report
> - status |= 2 if transaction.report.metrics["changes"][:total]
> > 0
> - status |= 4 if
> transaction.report.metrics["resources"][:failed] > 0
> + exit(transaction.report.to_exit_status)
> + else
> + exit(0)
> end
> - exit(status)
> rescue => detail
> if Puppet[:trace]
> puts detail.backtrace
> diff --git a/lib/puppet/application/puppetd.rb
> b/lib/puppet/application/puppetd.rb
> index 4799d55..0620fc2 100644
> --- a/lib/puppet/application/puppetd.rb
> +++ b/lib/puppet/application/puppetd.rb
> @@ -21,6 +21,7 @@ Puppet::Application.new(:puppetd) do
> {
> :waitforcert => 120, # Default to checking for certs every 5
> minutes
> :onetime => false,
> + :detailed_exitcodes => false,
> :verbose => false,
> :debug => false,
> :centrallogs => false,
> @@ -65,6 +66,10 @@ Puppet::Application.new(:puppetd) do
> options[:waitforcert] = 0 unless @explicit_waitforcert
> end
>
> + option("--detailed-exitcodes") do |arg|
> + options[:detailed_exitcodes] = true
> + end
> +
> option("--logdest DEST", "-l DEST") do |arg|
> begin
> Puppet::Util::Log.newdestination(arg)
> @@ -95,19 +100,25 @@ Puppet::Application.new(:puppetd) do
> unless options[:client]
> $stderr.puts "onetime is specified but there is no client"
> exit(43)
> + return
> end
>
> @daemon.set_signal_traps
>
> begin
> - �[email protected]
> + report = @agent.run
> rescue => detail
> if Puppet[:trace]
> puts detail.backtrace
> end
> Puppet.err detail.to_s
> end
> - exit(0)
> +
> + if not Puppet[:noop] and options[:detailed_exitcodes] then
> + exit(report.to_exit_status)
> + else
> + exit(0)
> + end
> end
>
> command(:main) do
> @@ -125,6 +136,7 @@ Puppet::Application.new(:puppetd) do
> Puppet.settings.handlearg("--no-daemonize")
> options[:verbose] = true
> options[:onetime] = true
> + options[:detailed_exitcodes] = true
> options[:waitforcert] = 0
> end
>
> diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
> index efda545..b63df07 100644
> --- a/lib/puppet/configurer.rb
> +++ b/lib/puppet/configurer.rb
> @@ -144,13 +144,17 @@ class Puppet::Configurer
>
> begin
> benchmark(:notice, "Finished catalog run") do
> - catalog.apply(options)
> + transaction = catalog.apply(options)
> + transaction.generate_report
> + report = transaction.report
> end
> + report
> rescue => detail
> puts detail.backtrace if Puppet[:trace]
> Puppet.err "Failed to apply catalog: %s" % detail
> + return
> end
> -
> + ensure
> # Now close all of our existing http connections, since there's no
> # reason to leave them lying open.
> Puppet::Network::HttpPool.clear_http_instances
> diff --git a/lib/puppet/transaction/report.rb
> b/lib/puppet/transaction/report.rb
> index e5b8650..5e9efb2 100644
> --- a/lib/puppet/transaction/report.rb
> +++ b/lib/puppet/transaction/report.rb
> @@ -83,5 +83,14 @@ class Puppet::Transaction::Report
> end
> return ret
> end
> -end
>
> + # Based on the contents of this report's metrics, compute a single number
> + # that represents the report. The resulting number is a bitmask where
> + # individual bits represent the presence of different metrics.
> + def to_exit_status
> + status = 0
> + status |= 2 if @metrics["changes"][:total] > 0
> + status |= 4 if @metrics["resources"][:failed] > 0
> + return status
> + end
> +end
> diff --git a/sbin/puppetd b/sbin/puppetd
> index 650a7e7..bf7d028 100755
> --- a/sbin/puppetd
> +++ b/sbin/puppetd
> @@ -8,7 +8,8 @@
> #
> # = Usage
> #
> -# puppetd [-D|--daemonize|--no-daemonize] [-d|--debug] [--disable]
> [--enable]
> +# puppetd [-D|--daemonize|--no-daemonize] [-d|--debug]
> +# [--detailed-exitcodes] [--disable] [--enable]
> # [-h|--help] [--fqdn <host name>] [-l|--logdest syslog|<file>|console]
> # [-o|--onetime] [--serve <handler>] [-t|--test] [--noop]
> # [-V|--version] [-v|--verbose] [-w|--waitforcert <seconds>]
> @@ -71,6 +72,12 @@
> # debug::
> # Enable full debugging.
> #
> +# detailed-exitcodes::
> +# Provide transaction information via exit codes. If this is enabled, an
> +# exit code of '2' means there were changes, and an exit code of '4' means
> +# that there were failures during the transaction. This option only makes
> +# sense in conjunction with --onetime.
> +#
> # disable::
> # Disable working on the local system. This puts a lock file in place,
> # causing +puppetd+ not to work on the system until the lock file is
> removed.
> diff --git a/spec/unit/application/puppet.rb b/spec/unit/application/puppet.rb
> index 3c11430..feee1ac 100755
> --- a/spec/unit/application/puppet.rb
> +++ b/spec/unit/application/puppet.rb
> @@ -283,52 +283,37 @@ describe "Puppet" do
> @puppet.main
> end
>
> - it "should generate a report if not noop" do
> - Puppet.stubs(:[]).with(:noop).returns(false)
> -
> �[email protected](:[]).with(:detailed_exitcodes).returns(true)
> - metrics = stub 'metrics', :[] => { :total => 10, :failed =>
> 0}
> - report = stub 'report', :metrics => metrics
> - �[email protected](:report).returns(report)
> -
> - �[email protected](:generate_report)
> -
> - �[email protected]
> - end
> -
> describe "with detailed_exitcodes" do
> - before :each do
> + it "should exit with report's computed exit status" do
> Puppet.stubs(:[]).with(:noop).returns(false)
>
> @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
> - end
> -
> - it "should exit with exit code of 2 if changes" do
> - report = stub 'report', :metrics => { "changes" =>
> {:total => 1}, "resources" => {:failed => 0} }
> - �[email protected](:generate_report).returns(report)
> + report = stub 'report', :to_exit_status => 666
> @transaction.stubs(:report).returns(report)
> - �[email protected](:exit).with(2)
> + �[email protected](:exit).with(666)
>
> @puppet.main
> end
>
> - it "should exit with exit code of 4 if failures" do
> - report = stub 'report', :metrics => { "changes" =>
> {:total => 0}, "resources" => {:failed => 1} }
> - �[email protected](:generate_report).returns(report)
> + it "should always exit with 0 if option is disabled" do
> + Puppet.stubs(:[]).with(:noop).returns(false)
> +
> �[email protected](:[]).with(:detailed_exitcodes).returns(false)
> + report = stub 'report', :to_exit_status => 666
> @transaction.stubs(:report).returns(report)
> - �[email protected](:exit).with(4)
> + �[email protected](:exit).with(0)
>
> @puppet.main
> end
>
> - it "should exit with exit code of 6 if changes and failures"
> do
> - report = stub 'report', :metrics => { "changes" =>
> {:total => 1}, "resources" => {:failed => 1} }
> - �[email protected](:generate_report).returns(report)
> + it "should always exit with 0 if --noop" do
> + Puppet.stubs(:[]).with(:noop).returns(true)
> +
> �[email protected](:[]).with(:detailed_exitcodes).returns(true)
> + report = stub 'report', :to_exit_status => 666
> @transaction.stubs(:report).returns(report)
> - �[email protected](:exit).with(6)
> + �[email protected](:exit).with(0)
>
> @puppet.main
> end
> end
> -
> end
>
> describe "the 'apply' command" do
> diff --git a/spec/unit/application/puppetd.rb
> b/spec/unit/application/puppetd.rb
> index d34ec9f..7e423d2 100755
> --- a/spec/unit/application/puppetd.rb
> +++ b/spec/unit/application/puppetd.rb
> @@ -205,6 +205,10 @@ describe "puppetd" do
> @puppetd.options.expects(:[]=).with(:onetime,true)
> @puppetd.setup_test
> end
> + it "should set options[:detailed_exitcodes] to true" do
> +
> �[email protected](:[]=).with(:detailed_exitcodes,true)
> + �[email protected]_test
> + end
> it "should set waitforcert to 0" do
> @puppetd.options.expects(:[]=).with(:waitforcert,0)
> @puppetd.setup_test
> @@ -453,6 +457,7 @@ describe "puppetd" do
>
> before :each do
> @puppetd.options.stubs(:[]).with(:client).returns(:client)
> +
> �[email protected](:[]).with(:detailed_exitcodes).returns(false)
> @puppetd.stubs(:exit).with(0)
> Puppet.stubs(:newservice)
> end
> @@ -484,6 +489,29 @@ describe "puppetd" do
> @puppetd.onetime
> end
>
> + describe "and --detailed-exitcodes" do
> + before :each do
> +
> �[email protected](:[]).with(:detailed_exitcodes).returns(true)
> + end
> +
> + it "should exit with report's computed exit status" do
> + Puppet.stubs(:[]).with(:noop).returns(false)
> + report = stub 'report', :to_exit_status => 666
> + �[email protected](:run).returns(report)
> + �[email protected](:exit).with(666)
> +
> + �[email protected]
> + end
> +
> + it "should always exit with 0 if --noop" do
> + Puppet.stubs(:[]).with(:noop).returns(true)
> + report = stub 'report', :to_exit_status => 666
> + �[email protected](:run).returns(report)
> + �[email protected](:exit).with(0)
> +
> + �[email protected]
> + end
> + end
> end
>
> describe "without --onetime" do
> diff --git a/spec/unit/transaction/report.rb b/spec/unit/transaction/report.rb
> index 1276573..646a3e0 100755
> --- a/spec/unit/transaction/report.rb
> +++ b/spec/unit/transaction/report.rb
> @@ -38,3 +38,26 @@ describe Puppet::Transaction::Report, " when being
> indirect" do
> Puppet::Util::Cacher.expire
> end
> end
> +
> +describe Puppet::Transaction::Report, " when computing exit status" do
> + it "should compute 2 if changes present" do
> + report = Puppet::Transaction::Report.new
> + report.newmetric("changes", {:total => 1})
> + report.newmetric("resources", {:failed => 0})
> + report.to_exit_status.should == 2
> + end
> +
> + it "should compute 4 if failures present" do
> + report = Puppet::Transaction::Report.new
> + report.newmetric("changes", {:total => 0})
> + report.newmetric("resources", {:failed => 1})
> + report.to_exit_status.should == 4
> + end
> +
> + it "should compute 6 if both changes and present" do
> + report = Puppet::Transaction::Report.new
> + report.newmetric("changes", {:total => 1})
> + report.newmetric("resources", {:failed => 1})
> + report.to_exit_status.should == 6
> + end
> +end
> --
> 1.6.4.3
>
>
> >
>
--
Rein Henrichs
http://reductivelabs.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
