summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLamont Granquist <lamont@scriptkiddie.org>2018-02-26 20:32:51 -0800
committerLamont Granquist <lamont@scriptkiddie.org>2018-02-26 20:32:51 -0800
commit7a09548c07dc473c220cd9b4def190d53b7e8c0f (patch)
treeb17eb4615f2c23a7b1f58a4947bf3161952e0fa2
parentbd43de8cb5b58dc7731d2945434aa2edf3d585e4 (diff)
downloadchef-7a09548c07dc473c220cd9b4def190d53b7e8c0f.tar.gz
Stop mixlib-cli default clobbering mixlib-config settingslcg/fix-default-precedence
The pre-14 precedence level is: 1. mixlib-cli setting 2. mixlib-cli default 3. mixlib-config setting 4. mixlib-config default This means that if an option has a mixlib-cli default that it cannot ever be set in the config file. This PR swaps 2+3 around: 1. mixlib-cli setting 2. mixlib-config setting 3. mixlib-cli default 4. mixlib-config default Now the mixlib-cli defaults still take precedence over mixlib-config defaults, but it is possible to set a value in config.rb if there's a mixlib-cli default setting (which creeps into the settings in hidden ways if you just use `boolean: true` in mixlib-cli). Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
-rw-r--r--lib/chef/application.rb116
-rw-r--r--lib/chef/knife.rb2
-rw-r--r--spec/unit/application/client_spec.rb2
-rw-r--r--spec/unit/application_spec.rb68
4 files changed, 139 insertions, 49 deletions
diff --git a/lib/chef/application.rb b/lib/chef/application.rb
index b20766c8b5..0d483fff0c 100644
--- a/lib/chef/application.rb
+++ b/lib/chef/application.rb
@@ -43,6 +43,11 @@ class Chef
# from failing due to permissions when launched as a less privileged user.
end
+ # Configure mixlib-cli to always separate defaults from user-supplied CLI options
+ def self.use_separate_defaults?
+ true
+ end
+
# Reconfigure the application. You'll want to override and super this method.
def reconfigure
configure_chef
@@ -70,33 +75,54 @@ class Chef
unless Chef::Platform.windows?
trap("QUIT") do
- Chef::Log.info("SIGQUIT received, call stack:\n " + caller.join("\n "))
+ log.info("SIGQUIT received, call stack:\n " + caller.join("\n "))
end
trap("HUP") do
- Chef::Log.info("SIGHUP received, reconfiguring")
+ log.info("SIGHUP received, reconfiguring")
reconfigure
end
end
end
def emit_warnings
- Chef::Log.warn "Chef::Config[:zypper_check_gpg] is set to false which disables security checking on zypper packages" unless Chef::Config[:zypper_check_gpg]
+ log.warn "chef_config[:zypper_check_gpg] is set to false which disables security checking on zypper packages" unless chef_config[:zypper_check_gpg]
end
# Parse configuration (options and config file)
def configure_chef
parse_options
load_config_file
- Chef::Config.export_proxies
- Chef::Config.init_openssl
- File.umask Chef::Config[:umask]
+ chef_config.export_proxies
+ chef_config.init_openssl
+ File.umask chef_config[:umask]
+ end
+
+ # @api private (test injection)
+ def chef_config
+ Chef::Config
+ end
+
+ # @api private (test injection)
+ def log
+ Chef::Log
+ end
+
+ def self.log
+ Chef::Log
+ end
+
+ # @api private (test injection)
+ def chef_configfetcher
+ Chef::ConfigFetcher
end
# Parse the config file
def load_config_file
- config_fetcher = Chef::ConfigFetcher.new(config[:config_file])
+ # apply the default cli options first
+ chef_config.merge!(default_config)
+ config_fetcher = chef_configfetcher.new(config[:config_file])
# Some config settings are derived relative to the config file path; if
# given as a relative path, this is computed relative to cwd, but
# chef-client will later chdir to root, so we need to get the absolute path
@@ -104,29 +130,29 @@ class Chef
config[:config_file] = config_fetcher.expanded_path
if config[:config_file].nil?
- Chef::Log.warn("No config file found or specified on command line, using command line options.")
+ log.warn("No config file found or specified on command line, using command line options.")
elsif config_fetcher.config_missing?
- Chef::Log.warn("*****************************************")
- Chef::Log.warn("Did not find config file: #{config[:config_file]}, using command line options.")
- Chef::Log.warn("*****************************************")
+ log.warn("*****************************************")
+ log.warn("Did not find config file: #{config[:config_file]}, using command line options.")
+ log.warn("*****************************************")
else
config_content = config_fetcher.read_config
apply_config(config_content, config[:config_file])
end
extra_config_options = config.delete(:config_option)
- Chef::Config.merge!(config)
+ chef_config.merge!(config)
apply_extra_config_options(extra_config_options)
end
def apply_extra_config_options(extra_config_options)
- Chef::Config.apply_extra_config_options(extra_config_options)
+ chef_config.apply_extra_config_options(extra_config_options)
rescue ChefConfig::UnparsableConfigOption => e
Chef::Application.fatal!(e.message)
end
def set_specific_recipes
if cli_arguments.respond_to?(:map)
- Chef::Config[:specific_recipes] =
+ chef_config[:specific_recipes] =
cli_arguments.map { |file| File.expand_path(file) }
end
end
@@ -150,26 +176,26 @@ class Chef
#
def configure_logging
configure_log_location
- Chef::Log.init(MonoLogger.new(Chef::Config[:log_location]))
+ log.init(MonoLogger.new(chef_config[:log_location]))
if want_additional_logger?
configure_stdout_logger
end
- Chef::Log.level = resolve_log_level
+ log.level = resolve_log_level
rescue StandardError => error
- Chef::Log.fatal("Failed to open or create log file at #{Chef::Config[:log_location]}: #{error.class} (#{error.message})")
+ log.fatal("Failed to open or create log file at #{chef_config[:log_location]}: #{error.class} (#{error.message})")
Chef::Application.fatal!("Aborting due to invalid 'log_location' configuration", error)
end
# Turn `log_location :syslog` and `log_location :win_evt` into the
# appropriate loggers.
def configure_log_location
- log_location = Chef::Config[:log_location]
+ log_location = chef_config[:log_location]
return unless log_location.respond_to?(:to_sym)
- Chef::Config[:log_location] =
+ chef_config[:log_location] =
case log_location.to_sym
- when :syslog then Chef::Log::Syslog.new
- when :win_evt then Chef::Log::WinEvt.new
+ when :syslog then log::Syslog.new
+ when :win_evt then log::WinEvt.new
else log_location # Probably a path; let MonoLogger sort it out
end
end
@@ -177,22 +203,22 @@ class Chef
# Based on config and whether or not STDOUT is a tty, should we setup a
# secondary logger for stdout?
def want_additional_logger?
- !( Chef::Config[:log_location].is_a?(IO) && Chef::Config[:log_location].tty? ) && !Chef::Config[:daemonize]
+ !( chef_config[:log_location].is_a?(IO) && chef_config[:log_location].tty? ) && !chef_config[:daemonize]
end
def configure_stdout_logger
stdout_logger = MonoLogger.new(STDOUT)
- stdout_logger.formatter = Chef::Log.logger.formatter
- Chef::Log.loggers << stdout_logger
+ stdout_logger.formatter = log.logger.formatter
+ log.loggers << stdout_logger
end
# Use of output formatters is assumed if `force_formatter` is set or if `force_logger` is not set
def using_output_formatter?
- Chef::Config[:force_formatter] || !Chef::Config[:force_logger]
+ chef_config[:force_formatter] || !chef_config[:force_logger]
end
def auto_log_level?
- Chef::Config[:log_level] == :auto
+ chef_config[:log_level] == :auto
end
# if log_level is `:auto`, convert it to :warn (when using output formatter)
@@ -205,13 +231,13 @@ class Chef
:info
end
else
- Chef::Config[:log_level]
+ chef_config[:log_level]
end
end
# Sets the default external encoding to UTF-8 (users can change this, but they shouldn't)
def configure_encoding
- Encoding.default_external = Chef::Config[:ruby_encoding]
+ Encoding.default_external = chef_config[:ruby_encoding]
end
# Called prior to starting the application, by the run method
@@ -259,7 +285,7 @@ class Chef
# win32-process gem exposes some form of :fork for Process
# class. So we are separately ensuring that the platform we're
# running on is not windows before forking.
- Chef::Config[:client_fork] && Process.respond_to?(:fork) && !Chef::Platform.windows?
+ chef_config[:client_fork] && Process.respond_to?(:fork) && !Chef::Platform.windows?
end
# Run chef-client once and then exit. If TERM signal is received, ignores the
@@ -267,7 +293,7 @@ class Chef
def run_with_graceful_exit_option
# Override the TERM signal.
trap("TERM") do
- Chef::Log.debug("SIGTERM received during converge," +
+ log.debug("SIGTERM received during converge," +
" finishing converge to exit normally (send SIGINT to terminate immediately)")
end
@@ -276,31 +302,31 @@ class Chef
end
def fork_chef_client
- Chef::Log.info "Forking chef instance to converge..."
+ log.info "Forking chef instance to converge..."
pid = fork do
# Want to allow forked processes to finish converging when
# TERM singal is received (exit gracefully)
trap("TERM") do
- Chef::Log.debug("SIGTERM received during converge," +
+ log.debug("SIGTERM received during converge," +
" finishing converge to exit normally (send SIGINT to terminate immediately)")
end
- client_solo = Chef::Config[:solo] ? "chef-solo" : "chef-client"
+ client_solo = chef_config[:solo] ? "chef-solo" : "chef-client"
$0 = "#{client_solo} worker: ppid=#{Process.ppid};start=#{Time.new.strftime("%R:%S")};"
begin
- Chef::Log.debug "Forked instance now converging"
+ log.debug "Forked instance now converging"
@chef_client.run
rescue Exception => e
- Chef::Log.error(e.to_s)
+ log.error(e.to_s)
exit Chef::Application.normalize_exit_code(e)
else
exit 0
end
end
- Chef::Log.debug "Fork successful. Waiting for new chef pid: #{pid}"
+ log.debug "Fork successful. Waiting for new chef pid: #{pid}"
result = Process.waitpid2(pid)
handle_child_exit(result)
- Chef::Log.debug "Forked instance successfully reaped (pid: #{pid})"
+ log.debug "Forked instance successfully reaped (pid: #{pid})"
true
end
@@ -316,11 +342,11 @@ class Chef
end
def apply_config(config_content, config_file_path)
- Chef::Config.from_string(config_content, config_file_path)
+ chef_config.from_string(config_content, config_file_path)
rescue Exception => error
- Chef::Log.fatal("Configuration error #{error.class}: #{error.message}")
+ log.fatal("Configuration error #{error.class}: #{error.message}")
filtered_trace = error.backtrace.grep(/#{Regexp.escape(config_file_path)}/)
- filtered_trace.each { |line| Chef::Log.fatal(" " + line ) }
+ filtered_trace.each { |line| log.fatal(" " + line ) }
Chef::Application.fatal!("Aborting due to error in '#{config_file_path}'", error)
end
@@ -343,9 +369,9 @@ class Chef
chef_stacktrace_out += message
Chef::FileCache.store("chef-stacktrace.out", chef_stacktrace_out)
- Chef::Log.fatal("Stacktrace dumped to #{Chef::FileCache.load("chef-stacktrace.out", false)}")
- Chef::Log.fatal("Please provide the contents of the stacktrace.out file if you file a bug report")
- Chef::Log.debug(message)
+ log.fatal("Stacktrace dumped to #{Chef::FileCache.load("chef-stacktrace.out", false)}")
+ log.fatal("Please provide the contents of the stacktrace.out file if you file a bug report")
+ log.debug(message)
true
end
@@ -355,12 +381,12 @@ class Chef
# Log a fatal error message to both STDERR and the Logger, exit the application
def fatal!(msg, err = nil)
- Chef::Log.fatal(msg)
+ log.fatal(msg)
Process.exit(normalize_exit_code(err))
end
def exit!(msg, err = nil)
- Chef::Log.debug(msg)
+ log.debug(msg)
Process.exit(normalize_exit_code(err))
end
end
diff --git a/lib/chef/knife.rb b/lib/chef/knife.rb
index 2853bf65dd..4e975c2b27 100644
--- a/lib/chef/knife.rb
+++ b/lib/chef/knife.rb
@@ -1,7 +1,7 @@
#
# Author:: Adam Jacob (<adam@chef.io>)
# Author:: Christopher Brown (<cb@chef.io>)
-# Copyright:: Copyright 2009-2017, Chef Software Inc.
+# Copyright:: Copyright 2009-2018, Chef Software Inc.
# License:: Apache License, Version 2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
diff --git a/spec/unit/application/client_spec.rb b/spec/unit/application/client_spec.rb
index 825a4e7aac..3f803f2033 100644
--- a/spec/unit/application/client_spec.rb
+++ b/spec/unit/application/client_spec.rb
@@ -1,6 +1,6 @@
#
# Author:: AJ Christensen (<aj@junglist.gen.nz>)
-# Copyright:: Copyright 2008-2016, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
# License:: Apache License, Version 2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 7981748962..273f605905 100644
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -1,7 +1,7 @@
#
# Author:: AJ Christensen (<aj@junglist.gen.nz>)
# Author:: Mark Mzyk (mmzyk@chef.io)
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
# License:: Apache License, Version 2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
@@ -171,7 +171,7 @@ describe Chef::Application do
@app.config[:config_file] = "/etc/chef/notfound"
end
it "should use the passed in command line options and defaults" do
- expect(Chef::Config).to receive(:merge!)
+ expect(Chef::Config).to receive(:merge!).at_least(:once)
@app.configure_chef
end
end
@@ -418,4 +418,68 @@ describe Chef::Application do
end
end
end
+
+ describe "merged config" do
+ class MyTestConfig < Chef::Config
+ extend Mixlib::Config
+
+ default :test_config_1, "config default"
+ default :test_config_2, "config default"
+ end
+
+ class MyAppClass < Chef::Application
+ # there's an implicit test here that mixlib-cli's separate_default_options is being inherited
+ option :test_config_2, long: "--test-config2 CONFIG", default: "cli default"
+ end
+
+ before(:each) do
+ MyTestConfig.reset
+ @original_argv = ARGV.dup
+ ARGV.clear
+ @app = MyAppClass.new
+ expect(@app).to receive(:chef_config).at_least(:once).and_return(MyTestConfig)
+ expect(Chef::ConfigFetcher).to receive(:new).and_return(fake_config_fetcher)
+ allow(@app).to receive(:log).and_return(instance_double(Mixlib::Log, warn: nil)) # ignorken
+ end
+
+ after(:each) do
+ ARGV.replace(@original_argv)
+ end
+
+ let(:fake_config_fetcher) { instance_double(Chef::ConfigFetcher, expanded_path: "/thisbetternotexist", :"config_missing?" => false, read_config: "" ) }
+
+ it "reading a mixlib-config default works" do
+ @app.parse_options
+ @app.load_config_file
+ expect(MyTestConfig[:test_config_1]).to eql("config default")
+ end
+
+ it "a mixlib-cli default overrides a mixlib-config default" do
+ @app.parse_options
+ @app.load_config_file
+ expect(MyTestConfig[:test_config_2]).to eql("cli default")
+ end
+
+ it "a set mixlib-config value overrides a mixlib-config default" do
+ expect(fake_config_fetcher).to receive(:read_config).and_return(%q{test_config_1 "config setting"})
+ @app.parse_options
+ @app.load_config_file
+ expect(MyTestConfig[:test_config_1]).to eql("config setting")
+ end
+
+ it "a set mixlib-config value overrides a mixlib-cli default" do
+ expect(fake_config_fetcher).to receive(:read_config).and_return(%q{test_config_2 "config setting"})
+ @app.parse_options
+ @app.load_config_file
+ expect(MyTestConfig[:test_config_2]).to eql("config setting")
+ end
+
+ it "a set mixlib-cli value overrides everything else" do
+ expect(fake_config_fetcher).to receive(:read_config).and_return(%q{test_config_2 "config setting"})
+ ARGV.replace("--test-config2 cli-setting".split)
+ @app.parse_options
+ @app.load_config_file
+ expect(MyTestConfig[:test_config_2]).to eql("cli-setting")
+ end
+ end
end