Permalink
Browse files

(#7749) Parse command line args and config file before loading app/face

This commit is mostly focused on solving the following chicken-and-egg
problem: the way the code worked before, the command_line class would
try to load an app before doing any parsing of the cli args or the
puppet config file.  However, we want to support being able to load apps
and faces from the libdir... and if your libdir is in a non-standard
location, which you've specified via the command line or config file, then
the command_line class would fail to find any apps that were in your
libdir and exit before it ever even looked at the cli args or the config
file.

To get around this, and to prepare for supporting the ability to run
faces without an application stub, we needed to move the parsing of
the cli args and config file to an earlier point in the lifecycle.

The major changes in this commit are:

* Move the initial parsing of the cli args and the config file out of
application.rb and into command_line.rb so that they happen before
puppet tries to locate apps/faces.

* Remove all of the "should_parse_config" and "should_not_parse_config"
options and tests from the various apps.  These are not relevant any
longer because we always need to parse the config file to make sure
we know where the libdir is before we try to load faces / apps.

* Introduce a new option parser implementation, to supplement and
perhaps eventually replace our use of the Ruby stdlib OptionParser.
The stdlib OptionParser has some very undesirable behaviors that
are not configurable; the new PuppetOptionParser is a wrapper
around an open source parser called "trollop"; it maintains the
same API that OptionParser exposes, but gives us complete control
over what is happening behind the scenes.

Conflicts:

	spec/unit/util/command_line_spec.rb
  • Loading branch information...
cprice
cprice committed Mar 14, 2012
1 parent 2fe5b25 commit cb3ce74744f842b0ec9fc69c39caa35a413f8e11
Showing with 566 additions and 298 deletions.
  1. +0 −1 ext/upload_facts.rb
  2. +19 −49 lib/puppet/application.rb
  3. +0 −1 lib/puppet/application/agent.rb
  4. +0 −2 lib/puppet/application/apply.rb
  5. +0 −1 lib/puppet/application/cert.rb
  6. +0 −2 lib/puppet/application/describe.rb
  7. +0 −1 lib/puppet/application/device.rb
  8. +0 −1 lib/puppet/application/doc.rb
  9. +0 −1 lib/puppet/application/face_base.rb
  10. +0 −2 lib/puppet/application/filebucket.rb
  11. +0 −1 lib/puppet/application/inspect.rb
  12. +0 −2 lib/puppet/application/kick.rb
  13. +0 −1 lib/puppet/application/master.rb
  14. +0 −1 lib/puppet/application/queue.rb
  15. +0 −2 lib/puppet/application/resource.rb
  16. +4 −4 lib/puppet/defaults.rb
  17. +11 −0 lib/puppet/util.rb
  18. +62 −1 lib/puppet/util/command_line.rb
  19. +41 −0 lib/puppet/util/command_line_utils/legacy_command_line.rb
  20. +34 −9 lib/puppet/util/command_line_utils/lib_trollop.rb
  21. +44 −0 lib/puppet/util/command_line_utils/option_parsers/stdlib_parser.rb
  22. +65 −0 lib/puppet/util/command_line_utils/option_parsers/trollop_parser.rb
  23. +24 −0 lib/puppet/util/command_line_utils/puppet_option_parser.rb
  24. +0 −39 lib/puppet/util/legacy_command_line.rb
  25. +22 −3 lib/puppet/util/logging.rb
  26. +1 −1 lib/puppet/util/run_mode.rb
  27. +12 −4 lib/puppet/util/settings.rb
  28. +0 −4 spec/unit/application/agent_spec.rb
  29. +0 −4 spec/unit/application/apply_spec.rb
  30. +0 −4 spec/unit/application/cert_spec.rb
  31. +0 −4 spec/unit/application/describe_spec.rb
  32. +0 −4 spec/unit/application/device_spec.rb
  33. +0 −4 spec/unit/application/doc_spec.rb
  34. +14 −1 spec/unit/application/face_base_spec.rb
  35. +0 −4 spec/unit/application/filebucket_spec.rb
  36. +0 −4 spec/unit/application/kick_spec.rb
  37. +14 −4 spec/unit/application/master_spec.rb
  38. +0 −4 spec/unit/application/queue_spec.rb
  39. +0 −4 spec/unit/application/resource_spec.rb
  40. +10 −74 spec/unit/application_spec.rb
  41. +101 −50 spec/unit/util/command_line_spec.rb
  42. +88 −0 spec/unit/util/command_line_utils/puppet_option_parser_spec.rb
View
@@ -11,7 +11,6 @@
require 'puppet/network/http_pool'
class Puppet::Application::UploadFacts < Puppet::Application
should_parse_config
run_mode :master
option('--debug', '-d')
View
@@ -174,19 +174,6 @@ def controlled_run(&block)
result
end
def should_parse_config
@parse_config = true
end
def should_not_parse_config
@parse_config = false
end
def should_parse_config?
@parse_config = true if ! defined?(@parse_config)
@parse_config
end
# used to declare code that handle an option
def option(*options, &block)
long = options.find { |opt| opt =~ /^--/ }.gsub(/^--(?:\[no-\])?([^ =]+).*$/, '\1' ).gsub('-','_')
@@ -230,7 +217,7 @@ def [](name)
end
#
# TODO: look into changing this into two methods (getter/setter)
# TODO cprice: look into changing this into two methods (getter/setter)
# --cprice 2012-03-06
#
@@ -259,10 +246,6 @@ def run_mode( mode_name = nil)
exit
end
def should_parse_config?
self.class.should_parse_config?
end
# override to execute code before running anything else
def preinit
end
@@ -291,15 +274,15 @@ def set_run_mode(mode)
$puppet_application_mode = @run_mode
$puppet_application_name = name
# XXXXXXXXXXX TODO: this is crap. need to refactor this so that applications have a hook to initialize
# XXXXXXXXXXX TODO cprice: this is crap. need to refactor this so that applications have a hook to initialize
# the settings that are specific to them; need to decide whether to formally specify the list of such
# settings, or just allow them to run willy-nilly.
if (mode.name == :master)
Puppet.settings.set_value(:facts_terminus, "yaml", :mutable_defaults)
end
## TODO: get rid of this whole block, push it to some kind of application-specific hook or something
## TODO cprice: get rid of this whole block, push it to some kind of application-specific hook or something
if Puppet.respond_to? :settings
# This is to reduce the amount of confusion in rspec
@@ -316,12 +299,11 @@ def set_run_mode(mode)
# This is the main application entry point
def run
exit_on_fail("initialize") { hook('preinit') { preinit } }
exit_on_fail("parse options") { hook('parse_options') { parse_options } }
exit_on_fail("parse configuration file") { Puppet.settings.parse } if should_parse_config?
exit_on_fail("prepare for execution") { hook('setup') { setup } }
exit_on_fail("initialize") { plugin_hook('preinit') { preinit } }
exit_on_fail("parse application options") { plugin_hook('parse_options') { parse_options } }
exit_on_fail("prepare for execution") { plugin_hook('setup') { setup } }
exit_on_fail("configure routes from #{Puppet[:route_file]}") { configure_indirector_routes }
exit_on_fail("run") { hook('run_command') { run_command } }
exit_on_fail("run") { plugin_hook('run_command') { run_command } }
end
def main
@@ -362,6 +344,11 @@ def parse_options
# Create an option parser
option_parser = OptionParser.new(self.class.banner)
# TODO cprice: document this better. We need to include the globals so that the app
# has an opportunity to override them.
# Might be able to make this a little more efficient by sharing the Parser object
# betwween the command line class and this one.
#
# Add all global options to it.
Puppet.settings.optparse_addargs([]).each do |option|
option_parser.on(*option) do |arg|
@@ -385,22 +372,11 @@ def parse_options
option_parser.parse!(self.command_line.args)
end
def handlearg(opt, arg)
# rewrite --[no-]option to --no-option if that's what was given
if opt =~ /\[no-\]/ and !arg
opt = opt.gsub(/\[no-\]/,'no-')
end
# otherwise remove the [no-] prefix to not confuse everybody
opt = opt.gsub(/\[no-\]/, '')
unless respond_to?(:handle_unknown) and send(:handle_unknown, opt, arg)
# Puppet.settings.handlearg doesn't handle direct true/false :-)
if arg.is_a?(FalseClass)
arg = "false"
elsif arg.is_a?(TrueClass)
arg = "true"
end
Puppet.settings.handlearg(opt, arg)
end
def handlearg(opt, val)
opt, val = Puppet::Util::CommandLine.clean_opt(opt, val)
send(:handle_unknown, opt, val) if respond_to?(:handle_unknown)
end
# this is used for testing
@@ -416,20 +392,14 @@ def help
"No help available for puppet #{name}"
end
private
def exit_on_fail(message, code = 1)
yield
rescue ArgumentError, RuntimeError, NotImplementedError => detail
Puppet.log_exception(detail, "Could not #{message}: #{detail}")
exit(code)
end
def hook(step,&block)
def plugin_hook(step,&block)
Puppet::Plugins.send("before_application_#{step}",:application_object => self)
x = yield
Puppet::Plugins.send("after_application_#{step}",:application_object => self, :return_value => x)
x
end
private :plugin_hook
end
end
@@ -2,7 +2,6 @@
class Puppet::Application::Agent < Puppet::Application
should_parse_config
run_mode :agent
attr_accessor :args, :agent, :daemon, :host
@@ -2,8 +2,6 @@
class Puppet::Application::Apply < Puppet::Application
should_parse_config
option("--debug","-d")
option("--execute EXECUTE","-e") do |arg|
options[:code] = arg
@@ -2,7 +2,6 @@
class Puppet::Application::Cert < Puppet::Application
should_parse_config
run_mode :master
attr_accessor :all, :ca, :digest, :signed
@@ -170,8 +170,6 @@ def list_providers(type)
class Puppet::Application::Describe < Puppet::Application
banner "puppet describe [options] [type]"
should_not_parse_config
option("--short", "-s", "Only list parameters without detail") do |arg|
options[:parameters] = false
end
@@ -4,7 +4,6 @@
class Puppet::Application::Device < Puppet::Application
should_parse_config
run_mode :agent
attr_accessor :args, :agent, :host
@@ -1,7 +1,6 @@
require 'puppet/application'
class Puppet::Application::Doc < Puppet::Application
should_not_parse_config
run_mode :master
attr_accessor :unknown_args, :manifest
@@ -4,7 +4,6 @@
require 'pp'
class Puppet::Application::FaceBase < Puppet::Application
should_parse_config
run_mode :agent
option("--debug", "-d") do |arg|
@@ -2,8 +2,6 @@
class Puppet::Application::Filebucket < Puppet::Application
should_not_parse_config
option("--bucket BUCKET","-b")
option("--debug","-d")
option("--local","-l")
@@ -2,7 +2,6 @@
class Puppet::Application::Inspect < Puppet::Application
should_parse_config
run_mode :agent
option("--debug","-d")
@@ -2,8 +2,6 @@
class Puppet::Application::Kick < Puppet::Application
should_not_parse_config
attr_accessor :hosts, :tags, :classes
option("--all","-a")
@@ -2,7 +2,6 @@
class Puppet::Application::Master < Puppet::Application
should_parse_config
run_mode :master
option("--debug", "-d")
@@ -2,7 +2,6 @@
require 'puppet/util'
class Puppet::Application::Queue < Puppet::Application
should_parse_config
attr_accessor :daemon
@@ -2,8 +2,6 @@
class Puppet::Application::Resource < Puppet::Application
should_not_parse_config
attr_accessor :host, :extra_params
def preinit
View
@@ -1,16 +1,16 @@
# !!!! TODO: get rid of all run_mode/application_name calls in here...
# !!!! TODO cprice: get rid of all run_mode/application_name calls in here...
# The majority of Puppet's configuration settings are set in this file.
module Puppet
# TODO: get rid of all Puppet.run_mode and Puppet.application_name from this file.
# TODO cprice: get rid of all Puppet.run_mode and Puppet.application_name from this file.
setdefaults(:main,
:confdir => [Puppet.run_mode.conf_dir, "The main Puppet configuration directory. The default for this setting is calculated based on the user. If the process
is running as root or the user that Puppet is supposed to run as, it defaults to a system directory, but if it's running as any other user,
it defaults to being in the user's home directory."],
:vardir => [Puppet.run_mode.var_dir, "Where Puppet stores dynamic and growing data. The default for this setting is calculated specially, like `confdir`_."],
### TODO: introduce a constant for this...
### TODO cprice: introduce a constant for this...
:name => ['apply', "The name of the application, if we are running as one. The
default is essentially $0 without the path or `.rb`."],
:run_mode => [Puppet.run_mode.name.to_s, "The effective 'run mode' of the application: master, agent, or user."]
@@ -143,7 +143,7 @@ module Puppet
:catalog_terminus => ["compiler", "Where to get node catalogs. This is useful to change if, for instance,
you'd like to pre-compile catalogs and store them in memcached or some other easily-accessed store."],
:facts_terminus => {
### TODO: re-wire the master to override this to yaml
### TODO cprice: re-wire the master to override this to yaml
:default => Puppet.application_name.to_s == "master" ? 'yaml' : 'facter',
#:default => 'facter',
:desc => "The node facts terminus.",
View
@@ -455,6 +455,17 @@ def replace_file(file, default_mode, &block)
end
module_function :replace_file
#TODO cprice: document
def exit_on_fail(message, code = 1)
yield
rescue ArgumentError, RuntimeError, NotImplementedError => detail
Puppet.log_exception(detail, "Could not #{message}: #{detail}")
exit(code)
end
module_function :exit_on_fail
#######################################################################################################
# Deprecated methods relating to process execution; these have been moved to Puppet::Util::Execution
#######################################################################################################
Oops, something went wrong.

0 comments on commit cb3ce74

Please sign in to comment.