Please review pull request #455: (#12404) Reduce redundant code in lib/puppet/application opened by (jeffweiss)
Description:
Reduce redundant code by defining Puppet::Application.logdest_option,
which can then be called from applications that need a log destination
instead of copying the 9 lines of code specific to creating a log
destination.
Reduce redundant code by defining Puppet::Application.setup_logs
to ease (and standardize) the log setup across the applications.
Reduce redundant code by moving setup_host into Puppet::Application.
Encapsulate logic for exiting if Puppet.settings.print_config?
Encapsulate logic for sorting type providers in Puppet::Application::Describe.
Modify tests to use standard option :setdest.
Modify tests to mock options :setdest, :debug, and :verbose to
handle standardized checked against each option in consolidated
setup_logs method.
- Opened: Fri Feb 03 08:42:00 UTC 2012
- Based on: puppetlabs:master (ca50b7cd625d654265138ca07106317180ec4c04)
- Requested merge: jeffweiss:ticket/master/12404_reduce_redundant_code_in_lib/puppet/application (ede552878deb980501c3e51a6115e4286e75cd6e)
Diff follows:
diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index c940d06..ee39fbb 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -238,6 +238,18 @@ def run_mode( mode_name = nil)
require 'puppet/util/run_mode'
@run_mode = Puppet::Util::RunMode[ mode_name || :user ]
end
+
+ def logdest_option
+ option("--logdest DEST", "-l DEST") do |arg|
+ begin
+ Puppet::Util::Log.newdestination(arg)
+ options[:setdest] = true
+ rescue => detail
+ puts detail.backtrace if Puppet[:debug]
+ $stderr.puts detail.to_s
+ end
+ end
+ end
end
attr_reader :options, :command_line
@@ -318,17 +330,39 @@ def run_command
end
def setup
- # Handle the logging settings
- if options[:debug] or options[:verbose]
- Puppet::Util::Log.newdestination(:console)
- if options[:debug]
- Puppet::Util::Log.level = :debug
- else
- Puppet::Util::Log.level = :info
- end
- end
+ setup_logs :console => :requires_explicit_option
+ end
- Puppet::Util::Log.newdestination(:syslog) unless options[:setdest]
+ # Handle the logging settings.
+ def setup_logs(params = {})
+
+ explicit = params[:console] == :requires_explicit_option
+ setdest = options[:setdest]
+ chatty = (options[:debug] or options[:verbose])
+
+ Puppet::Util::Log.newdestination(:console) if explicit and chatty
+ Puppet::Util::Log.newdestination(:console) unless (explicit or setdest)
+
+ set_log_level if chatty
+
+ Puppet::Util::Log.newdestination(:syslog) if explicit and not setdest
+ end
+
+ def set_log_level
+ Puppet::Util::Log.level = options[:debug] ? :debug : :info
+ end
+
+ def setup_central_logs
+ logdest = args[:Server]
+
+ logdest += ":" + args[:Port] if args.include?(:Port)
+ Puppet::Util::Log.newdestination(logdest)
+ end
+
+ def setup_host(fingerprint = false)
+ @host = Puppet::SSL::Host.new
+ waitforcert = options[:waitforcert] || (Puppet[:onetime] ? 0 : Puppet[:waitforcert])
+ cert = @host.wait_for_cert(waitforcert) unless fingerprint
end
def configure_indirector_routes
@@ -407,6 +441,10 @@ def exit_on_fail(message, code = 1)
$stderr.puts "Could not #{message}: #{detail}"
exit(code)
end
+
+ def exit_if_print_configs
+ exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
+ end
def hook(step,&block)
Puppet::Plugins.send("before_application_#{step}",:application_object => self)
diff --git a/lib/puppet/application/agent.rb b/lib/puppet/application/agent.rb
index 191b3bf..61311d0 100644
--- a/lib/puppet/application/agent.rb
+++ b/lib/puppet/application/agent.rb
@@ -1,7 +1,6 @@
require 'puppet/application'
class Puppet::Application::Agent < Puppet::Application
-
should_parse_config
run_mode :agent
@@ -62,16 +61,8 @@ def preinit
option("--detailed-exitcodes") do |arg|
options[:detailed_exitcodes] = true
end
-
- option("--logdest DEST", "-l DEST") do |arg|
- begin
- Puppet::Util::Log.newdestination(arg)
- options[:setdest] = true
- rescue => detail
- puts detail.backtrace if Puppet[:debug]
- $stderr.puts detail.to_s
- end
- end
+
+ logdest_option
option("--waitforcert WAITFORCERT", "-w") do |arg|
options[:waitforcert] = arg.to_i
@@ -361,20 +352,6 @@ def setup_test
options[:detailed_exitcodes] = true
end
- # Handle the logging settings.
- def setup_logs
- if options[:debug] or options[:verbose]
- Puppet::Util::Log.newdestination(:console)
- if options[:debug]
- Puppet::Util::Log.level = :debug
- else
- Puppet::Util::Log.level = :info
- end
- end
-
- Puppet::Util::Log.newdestination(:syslog) unless options[:setdest]
- end
-
def enable_disable_client(agent)
if options[:enable]
agent.enable
@@ -397,12 +374,6 @@ def setup_listen
@daemon.server = server
end
- def setup_host
- @host = Puppet::SSL::Host.new
- waitforcert = options[:waitforcert] || (Puppet[:onetime] ? 0 : Puppet[:waitforcert])
- cert = @host.wait_for_cert(waitforcert) unless options[:fingerprint]
- end
-
def setup_agent
# We need tomake the client either way, we just don't start it
# if --no-client is set.
@@ -418,7 +389,7 @@ def setup_agent
# waitforcert happens.
@daemon.daemonize if Puppet[:daemonize]
- setup_host
+ setup_host
@objects = []
@@ -435,9 +406,9 @@ def setup_agent
def setup
setup_test if options[:test]
- setup_logs
+ setup_logs :console => :requires_explicit_option
- exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
+ exit_if_print_configs
args[:Server] = Puppet[:server]
if options[:fqdn]
@@ -445,12 +416,7 @@ def setup
Puppet[:certname] = options[:fqdn]
end
- if options[:centrallogs]
- logdest = args[:Server]
-
- logdest += ":" + args[:Port] if args.include?(:Port)
- Puppet::Util::Log.newdestination(logdest)
- end
+ setup_central_logs if options[:centrallogs]
Puppet.settings.use :main, :agent, :ssl
@@ -479,7 +445,7 @@ def setup
unless options[:fingerprint]
setup_agent
else
- setup_host
+ setup_host options[:fingerprint]
end
end
end
diff --git a/lib/puppet/application/apply.rb b/lib/puppet/application/apply.rb
index 382ee83..cdc5750 100644
--- a/lib/puppet/application/apply.rb
+++ b/lib/puppet/application/apply.rb
@@ -1,7 +1,6 @@
require 'puppet/application'
class Puppet::Application::Apply < Puppet::Application
-
should_parse_config
option("--debug","-d")
@@ -25,14 +24,7 @@ class Puppet::Application::Apply < Puppet::Application
options[:catalog] = arg
end
- option("--logdest LOGDEST", "-l") do |arg|
- begin
- Puppet::Util::Log.newdestination(arg)
- options[:logset] = true
- rescue => detail
- $stderr.puts detail.to_s
- end
- end
+ logdest_option
option("--parseonly") do
puts "--parseonly has been removed. Please use 'puppet parser validate <manifest>'"
@@ -230,9 +222,8 @@ def main
end
def setup
- exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
-
- Puppet::Util::Log.newdestination(:console) unless options[:logset]
+ exit_if_print_configs
+ setup_logs
client = nil
server = nil
@@ -244,12 +235,6 @@ def setup
# we want the last report to be persisted locally
Puppet::Transaction::Report.indirection.cache_class = :yaml
- if options[:debug]
- Puppet::Util::Log.level = :debug
- elsif options[:verbose]
- Puppet::Util::Log.level = :info
- end
-
# Make pluginsync local
Puppet[:pluginsource] = 'puppet:///plugins'
end
diff --git a/lib/puppet/application/cert.rb b/lib/puppet/application/cert.rb
index 7b14311..1c3ef88 100644
--- a/lib/puppet/application/cert.rb
+++ b/lib/puppet/application/cert.rb
@@ -197,8 +197,8 @@ def main
def setup
require 'puppet/ssl/certificate_authority'
- exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
-
+ exit_if_print_configs
+
Puppet::Util::Log.newdestination :console
if [:generate, :destroy].include? subcommand
diff --git a/lib/puppet/application/describe.rb b/lib/puppet/application/describe.rb
index 8ce20b6..e77ab72 100644
--- a/lib/puppet/application/describe.rb
+++ b/lib/puppet/application/describe.rb
@@ -150,18 +150,18 @@ def list_attrs(type, attrs)
end
def format_providers(type)
- type.providers.sort { |a,b|
- a.to_s <=> b.to_s
- }.each { |prov|
+ sort_providers(type).each { |prov|
puts "\n- **#{prov}**"
puts @format.wrap(type.provider(prov).doc, :indent => 4, :scrub => true)
}
end
+ def sort_providers(type)
+ type.providers.sort { |a,b| a.to_s <=> b.to_s }
+ end
+
def list_providers(type)
- list = type.providers.sort { |a,b|
- a.to_s <=> b.to_s
- }.join(", ")
+ list = sort_providers(type).join(", ")
puts @format.wrap(list, :indent => 4)
end
diff --git a/lib/puppet/application/device.rb b/lib/puppet/application/device.rb
index 89d597d..d423cff 100644
--- a/lib/puppet/application/device.rb
+++ b/lib/puppet/application/device.rb
@@ -1,9 +1,7 @@
require 'puppet/application'
require 'puppet/util/network_device'
-
class Puppet::Application::Device < Puppet::Application
-
should_parse_config
run_mode :agent
@@ -38,15 +36,7 @@ def preinit
options[:detailed_exitcodes] = true
end
- option("--logdest DEST", "-l DEST") do |arg|
- begin
- Puppet::Util::Log.newdestination(arg)
- options[:setdest] = true
- rescue => detail
- puts detail.backtrace if Puppet[:debug]
- $stderr.puts detail.to_s
- end
- end
+ logdest_option
option("--waitforcert WAITFORCERT", "-w") do |arg|
options[:waitforcert] = arg.to_i
@@ -171,8 +161,9 @@ def main
Puppet.info "starting applying configuration to #{device.name} at #{device.url}"
# override local $vardir and $certname
- Puppet.settings.set_value(:confdir, ::File.join(Puppet[:devicedir], device.name), :cli)
- Puppet.settings.set_value(:vardir, ::File.join(Puppet[:devicedir], device.name), :cli)
+ device_dir = ::File.join Puppet[:devicedir], device.name
+ Puppet.settings.set_value(:confdir, device_dir, :cli)
+ Puppet.settings.set_value(:vardir, device_dir, :cli)
Puppet.settings.set_value(:certname, device.name, :cli)
# this will reload and recompute default settings and create the devices sub vardir, or we hope so :-)
@@ -201,36 +192,11 @@ def main
end
end
- # Handle the logging settings.
- def setup_logs
- if options[:debug] or options[:verbose]
- Puppet::Util::Log.newdestination(:console)
- if options[:debug]
- Puppet::Util::Log.level = :debug
- else
- Puppet::Util::Log.level = :info
- end
- end
-
- Puppet::Util::Log.newdestination(:syslog) unless options[:setdest]
- end
-
- def setup_host
- @host = Puppet::SSL::Host.new
- waitforcert = options[:waitforcert] || (Puppet[:onetime] ? 0 : Puppet[:waitforcert])
- cert = @host.wait_for_cert(waitforcert)
- end
-
def setup
- setup_logs
+ setup_logs :console => :requires_explicit_option
args[:Server] = Puppet[:server]
- if options[:centrallogs]
- logdest = args[:Server]
-
- logdest += ":" + args[:Port] if args.include?(:Port)
- Puppet::Util::Log.newdestination(logdest)
- end
+ setup_central_logs if options[:centrallogs]
Puppet.settings.use :main, :agent, :device, :ssl
diff --git a/lib/puppet/application/doc.rb b/lib/puppet/application/doc.rb
index d2683b0..4bc5eb3 100644
--- a/lib/puppet/application/doc.rb
+++ b/lib/puppet/application/doc.rb
@@ -1,5 +1,4 @@
require 'puppet/application'
-
class Puppet::Application::Doc < Puppet::Application
should_not_parse_config
run_mode :master
@@ -263,12 +262,7 @@ def setup_rdoc(dummy_argument=:work_arround_for_ruby_GC_bug)
# Handle the logging settings.
if options[:debug] or options[:verbose]
- if options[:debug]
- Puppet::Util::Log.level = :debug
- else
- Puppet::Util::Log.level = :info
- end
-
+ set_log_level
Puppet::Util::Log.newdestination(:console)
end
end
diff --git a/lib/puppet/application/filebucket.rb b/lib/puppet/application/filebucket.rb
index f78e937..8dfec44 100644
--- a/lib/puppet/application/filebucket.rb
+++ b/lib/puppet/application/filebucket.rb
@@ -170,7 +170,7 @@ def setup
# Now parse the config
Puppet.parse_config
- exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
+ exit_if_print_configs
require 'puppet/file_bucket/dipper'
begin
diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb
index 5413a3d..15ec315 100644
--- a/lib/puppet/application/inspect.rb
+++ b/lib/puppet/application/inspect.rb
@@ -1,21 +1,13 @@
require 'puppet/application'
class Puppet::Application::Inspect < Puppet::Application
-
should_parse_config
run_mode :agent
option("--debug","-d")
option("--verbose","-v")
- option("--logdest LOGDEST", "-l") do |arg|
- begin
- Puppet::Util::Log.newdestination(arg)
- options[:logset] = true
- rescue => detail
- $stderr.puts detail.to_s
- end
- end
+ logdest_option
def help
<<-HELP
@@ -80,26 +72,20 @@ def help
end
def setup
- exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
-
+ exit_if_print_configs
+
raise "Inspect requires reporting to be enabled. Set report=true in puppet.conf to enable reporting." unless Puppet[:report]
@report = Puppet::Transaction::Report.new("inspect")
Puppet::Util::Log.newdestination(@report)
- Puppet::Util::Log.newdestination(:console) unless options[:logset]
+ setup_logs
Signal.trap(:INT) do
$stderr.puts "Exiting"
exit(1)
end
- if options[:debug]
- Puppet::Util::Log.level = :debug
- elsif options[:verbose]
- Puppet::Util::Log.level = :info
- end
-
Puppet::Transaction::Report.indirection.terminus_class = :rest
Puppet::Resource::Catalog.indirection.terminus_class = :yaml
end
diff --git a/lib/puppet/application/kick.rb b/lib/puppet/application/kick.rb
index 72f0608..f551fa9 100644
--- a/lib/puppet/application/kick.rb
+++ b/lib/puppet/application/kick.rb
@@ -1,7 +1,6 @@
require 'puppet/application'
class Puppet::Application::Kick < Puppet::Application
-
should_not_parse_config
attr_accessor :hosts, :tags, :classes
@@ -287,12 +286,8 @@ def preinit
end
def setup
- if options[:debug]
- Puppet::Util::Log.level = :debug
- else
- Puppet::Util::Log.level = :info
- end
-
+ set_log_level
+
# Now parse the config
Puppet.parse_config
diff --git a/lib/puppet/application/master.rb b/lib/puppet/application/master.rb
index 553e279..6b2075a 100644
--- a/lib/puppet/application/master.rb
+++ b/lib/puppet/application/master.rb
@@ -1,7 +1,5 @@
require 'puppet/application'
-
class Puppet::Application::Master < Puppet::Application
-
should_parse_config
run_mode :master
@@ -14,16 +12,8 @@ class Puppet::Application::Master < Puppet::Application
option("--compile host", "-c host") do |arg|
options[:node] = arg
end
-
- option("--logdest DEST", "-l DEST") do |arg|
- begin
- Puppet::Util::Log.newdestination(arg)
- options[:setdest] = true
- rescue => detail
- puts detail.backtrace if Puppet[:debug]
- $stderr.puts detail.to_s
- end
- end
+
+ logdest_option
option("--parseonly") do
puts "--parseonly has been removed. Please use 'puppet parser validate <manifest>'"
@@ -206,12 +196,8 @@ def setup
# Handle the logging settings.
if options[:debug] or options[:verbose]
- if options[:debug]
- Puppet::Util::Log.level = :debug
- else
- Puppet::Util::Log.level = :info
- end
-
+ set_log_level
+
unless Puppet[:daemonize] or options[:rack]
Puppet::Util::Log.newdestination(:console)
options[:setdest] = true
@@ -220,8 +206,8 @@ def setup
Puppet::Util::Log.newdestination(:syslog) unless options[:setdest]
- exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
-
+ exit_if_print_configs
+
Puppet.settings.use :main, :master, :ssl, :metrics
# Cache our nodes in yaml. Currently not configurable.
diff --git a/lib/puppet/application/queue.rb b/lib/puppet/application/queue.rb
index 2a9d3e7..e95e849 100644
--- a/lib/puppet/application/queue.rb
+++ b/lib/puppet/application/queue.rb
@@ -36,6 +36,8 @@ def preinit
option("--debug","-d")
option("--verbose","-v")
+ logdest_option
+
def help
<<-HELP
@@ -107,27 +109,7 @@ def help
HELP
end
-
- option("--logdest DEST", "-l DEST") do |arg|
- begin
- Puppet::Util::Log.newdestination(arg)
- options[:setdest] = true
- rescue => detail
- puts detail.backtrace if Puppet[:debug]
- $stderr.puts detail.to_s
- end
- end
-
- option("--logdest DEST", "-l DEST") do |arg|
- begin
- Puppet::Util::Log.newdestination(arg)
- options[:setdest] = true
- rescue => detail
- puts detail.backtrace if Puppet[:debug]
- $stderr.puts detail.to_s
- end
- end
-
+
def main
require 'puppet/indirector/catalog/queue' # provides Puppet::Indirector::Queue.subscribe
Puppet.notice "Starting puppetqd #{Puppet.version}"
@@ -148,28 +130,15 @@ def main
Thread.list.each { |thread| thread.join }
end
- # Handle the logging settings.
- def setup_logs
- if options[:debug] or options[:verbose]
- Puppet::Util::Log.newdestination(:console)
- if options[:debug]
- Puppet::Util::Log.level = :debug
- else
- Puppet::Util::Log.level = :info
- end
- end
- Puppet::Util::Log.newdestination(:syslog) unless options[:setdest]
- end
-
def setup
unless Puppet.features.stomp?
raise ArgumentError, "Could not load the 'stomp' library, which must be present for queueing to work. You must install the required library."
end
- setup_logs
-
- exit(Puppet.settings.print_configs ? 0 : 1) if Puppet.settings.print_configs?
+ setup_logs :console => :requires_explicit_option
+ exit_if_print_configs
+
require 'puppet/resource/catalog'
Puppet::Resource::Catalog.indirection.terminus_class = :store_configs
diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb
index c9a84f3..6b80bc9 100644
--- a/lib/puppet/application/resource.rb
+++ b/lib/puppet/application/resource.rb
@@ -1,7 +1,6 @@
require 'puppet/application'
class Puppet::Application::Resource < Puppet::Application
-
should_not_parse_config
attr_accessor :host, :extra_params
@@ -152,15 +151,9 @@ def main
end
def setup
- Puppet::Util::Log.newdestination(:console)
+ setup_logs
Puppet.parse_config
-
- if options[:debug]
- Puppet::Util::Log.level = :debug
- elsif options[:verbose]
- Puppet::Util::Log.level = :info
- end
end
private
diff --git a/spec/unit/application/apply_spec.rb b/spec/unit/application/apply_spec.rb
index bf33dab..01858e5 100755
--- a/spec/unit/application/apply_spec.rb
+++ b/spec/unit/application/apply_spec.rb
@@ -49,7 +49,7 @@
end
it "should put the logset options to true" do
- @apply.options.expects(:[]=).with(:logset,true)
+ @apply.options.expects(:[]=).with(:setdest,true)
@apply.handle_logdest("console")
end
diff --git a/spec/unit/application/inspect_spec.rb b/spec/unit/application/inspect_spec.rb
index 58eff02..b9acc1d 100755
--- a/spec/unit/application/inspect_spec.rb
+++ b/spec/unit/application/inspect_spec.rb
@@ -36,7 +36,7 @@
describe "when executing" do
before :each do
Puppet[:report] = true
- @inspect.options[:logset] = true
+ @inspect.options[:setdest] = true
Puppet::Transaction::Report::Rest.any_instance.stubs(:save)
@inspect.setup
end
diff --git a/spec/unit/application/resource_spec.rb b/spec/unit/application/resource_spec.rb
index 2c173f9..856c64b 100755
--- a/spec/unit/application/resource_spec.rb
+++ b/spec/unit/application/resource_spec.rb
@@ -73,6 +73,9 @@
before :each do
Puppet::Log.stubs(:newdestination)
Puppet.stubs(:parse_config)
+ @resource_app.options.stubs(:[]).with(:setdest).returns(false)
+ @resource_app.options.stubs(:[]).with(:debug).returns(nil)
+ @resource_app.options.stubs(:[]).with(:verbose).returns(nil)
end
it "should set console as the log destination" do
-- 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.
