CHEF-4725: Validate 'log_location' setting on log initialization

Previously, a Mixlib::Config setter method verified that 'log_location'
was set to a writeable file or a valid IO stream. Due to the way
Mixlib::Config handles `default` and `merge!`, this setter method would
not fire if the 'log_location' was left unconfigured or if it was set
on the command line, resulting in inconsistent error handling.

This commit moves the validation logic out of the configuration layer
and into the log initializion layer. This ensures that error handling is
consistent, regardless of where the 'log_location' setting is
configured. Validation logic is also simplified, relying on the
MonoLogger class to open and configure any necessary IO streams.

5 files changed, 52 insertions, 56 deletions

diff --git a/lib/chef/application.rb b/lib/chef/application.rb
index 98bdedff7f..4e6b29cec6 100644
--- a/lib/chef/application.rb
+++ b/lib/chef/application.rb
@@ -118,6 +118,9 @@ class Chef::Application
       configure_stdout_logger
     end
     Chef::Log.level = resolve_log_level
+    rescue => error
+      Chef::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", 2)
   end
 
   def configure_stdout_logger
diff --git a/lib/chef/config.rb b/lib/chef/config.rb
index f8bfa3ac21..f5cba9e0ed 100644
--- a/lib/chef/config.rb
+++ b/lib/chef/config.rb
@@ -127,26 +127,6 @@ class Chef
     # properly.
     configurable(:daemonize).writes_value { |v| v }
 
-    # Override the config dispatch to set the value of log_location configuration option
-    #
-    # === Parameters
-    # location<IO||String>:: Logging location as either an IO stream or string representing log file path
-    #
-    config_attr_writer :log_location do |location|
-      if location.respond_to? :sync=
-        location.sync = true
-        location
-      elsif location.respond_to? :to_str
-        begin
-          f = File.new(location.to_str, "a")
-          f.sync = true
-        rescue Errno::ENOENT
-          raise Chef::Exceptions::ConfigurationError, "Failed to open or create log file at #{location.to_str}"
-        end
-        f
-      end
-    end
-
     # The root where all local chef object data is stored.  cookbooks, data bags,
     # environments are all assumed to be in separate directories under this.
     # chef-solo uses these directories for input data.  knife commands
@@ -299,6 +279,9 @@ class Chef
     # logger is the primary mode of output, and the log level is set to :info
     default :log_level, :auto
 
+    # Logging location as either an IO stream or string representing log file path
+    default :log_location, STDOUT
+
     # Using `force_formatter` causes chef to default to formatter output when STDOUT is not a tty
     default :force_formatter, false
 
@@ -310,7 +293,6 @@ class Chef
     default :interval, nil
     default :once, nil
     default :json_attribs, nil
-    default :log_location, STDOUT
     # toggle info level log items that can create a lot of output
     default :verbose_logging, true
     default :node_name, nil
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 1606c82531..426dbd5242 100644
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -163,6 +163,13 @@ describe Chef::Application do
       @app.configure_logging
     end
 
+    it "should raise fatals if log location is invalid" do
+      Chef::Config[:log_location] = "/tmp/non-existing-dir/logfile"
+      Chef::Log.should_receive(:fatal).at_least(:once)
+      Process.should_receive(:exit)
+      @app.configure_logging
+    end
+
     shared_examples_for "log_level_is_auto" do
       context "when STDOUT is to a tty" do
         before do
@@ -306,11 +313,6 @@ describe Chef::Application do
         @config_file.unlink
       end
 
-      it "should raise informative fatals for missing log file dir" do
-        create_config_file('log_location "/tmp/non-existing-dir/logfile"')
-        raises_informative_fatals_on_configure_chef
-      end
-
       it "should raise informative fatals for badly written config" do
         create_config_file("text that should break the config parsing")
         raises_informative_fatals_on_configure_chef
diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb
index 77870e84d8..2bf003bbdb 100644
--- a/spec/unit/config_spec.rb
+++ b/spec/unit/config_spec.rb
@@ -121,29 +121,6 @@ describe Chef::Config do
 
   end
 
-  describe "config attribute writer: log_method=" do
-    describe "when given an object that responds to sync= e.g. IO" do
-      it "should configure itself to use the IO as log_location" do
-        Chef::Config.log_location = STDOUT
-        Chef::Config.log_location.should == STDOUT
-      end
-    end
-
-    describe "when given an object that is stringable (to_str)" do
-      before do
-        @mockfile = mock("File", :path => "/var/log/chef/client.log", :sync= => true)
-        File.should_receive(:new).
-          with("/var/log/chef/client.log", "a").
-          and_return(@mockfile)
-      end
-
-      it "should configure itself to use a File object based upon the String" do
-        Chef::Config.log_location = "/var/log/chef/client.log"
-        Chef::Config.log_location.path.should == "/var/log/chef/client.log"
-      end
-    end
-  end
-
   describe "class method: plaform_specific_path" do
     it "should return given path on non-windows systems" do
       platform_mock :unix do
@@ -354,13 +331,6 @@ describe Chef::Config do
     end
   end
 
-  describe "Chef::Config[:log_location]" do
-    it "raises ConfigurationError when log_location directory is missing" do
-      missing_path = "/tmp/non-existing-dir/file"
-      expect{Chef::Config.log_location = missing_path}.to raise_error Chef::Exceptions::ConfigurationError
-    end
-  end
-
   describe "Chef::Config[:event_handlers]" do
     it "sets a event_handlers to an empty array by default" do
       Chef::Config[:event_handlers].should eq([])
diff --git a/spec/unit/monologger_spec.rb b/spec/unit/monologger_spec.rb
new file mode 100644
index 0000000000..38b23f22c6
--- /dev/null
+++ b/spec/unit/monologger_spec.rb
@@ -0,0 +1,39 @@
+#
+# Copyright:: Copyright (c) 2014 Opscode, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'chef/monologger'
+require 'tempfile'
+require 'spec_helper'
+
+describe MonoLogger do
+  describe "when given an object that responds to write and close e.g. IO" do
+    it "should use the object directly" do
+      stream = StringIO.new
+      MonoLogger.new(stream).fatal("Houston, we've had a problem.")
+      stream.string.should =~ /Houston, we've had a problem./
+    end
+  end
+
+  describe "when given an object that is stringable (to_str)" do
+    it "should open a File object with the given path" do
+      temp_file = Tempfile.new("rspec-monologger-log")
+      temp_file.close
+      MonoLogger.new(temp_file.path).fatal("Do, or do not. There is no try.")
+      File.read(temp_file.path).should =~ /Do, or do not. There is no try./
+    end
+  end
+end