Merge branch 'CHEF-4725'

7 files changed, 59 insertions, 60 deletions

diff --git a/lib/chef/application.rb b/lib/chef/application.rb
index 98bdedff7f..04e88de2ce 100644
--- a/lib/chef/application.rb
+++ b/lib/chef/application.rb
@@ -118,11 +118,13 @@ class Chef::Application
       configure_stdout_logger
     end
     Chef::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})")
+    Chef::Application.fatal!("Aborting due to invalid 'log_location' configuration", 2)
   end
 
   def configure_stdout_logger
     stdout_logger = MonoLogger.new(STDOUT)
-    STDOUT.sync = true
     stdout_logger.formatter = Chef::Log.logger.formatter
     Chef::Log.loggers <<  stdout_logger
   end
diff --git a/lib/chef/application/windows_service.rb b/lib/chef/application/windows_service.rb
index 4c99e50b0e..bea5e9fcdc 100644
--- a/lib/chef/application/windows_service.rb
+++ b/lib/chef/application/windows_service.rb
@@ -233,7 +233,6 @@ class Chef
 
       def configure_stdout_logger
         stdout_logger = MonoLogger.new(STDOUT)
-        STDOUT.sync = true
         stdout_logger.formatter = Chef::Log.logger.formatter
         Chef::Log.loggers <<  stdout_logger
       end
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/lib/chef/monologger.rb b/lib/chef/monologger.rb
index fed60514d7..464b21bdd3 100644
--- a/lib/chef/monologger.rb
+++ b/lib/chef/monologger.rb
@@ -48,9 +48,9 @@ class MonoLogger < Logger
         @dev = log
       else
         @dev = open_logfile(log)
-        @dev.sync = true
         @filename = log
       end
+      @dev.sync = true
     end
 
     def write(message)
@@ -75,7 +75,6 @@ class MonoLogger < Logger
 
     def create_logfile(filename)
       logdev = open(filename, (File::WRONLY | File::APPEND | File::CREAT))
-      logdev.sync = true
       add_log_header(logdev)
       logdev
     end
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..3babc29218
--- /dev/null
+++ b/spec/unit/monologger_spec.rb
@@ -0,0 +1,45 @@
+#
+# 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
+  it "should disable buffering when passed an IO stream" do
+    STDOUT.sync = false
+    MonoLogger.new(STDOUT)
+    STDOUT.sync.should == true
+  end
+
+  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