windows service uses log filemwrock/service

    location from config if none is given on commandline

3 files changed, 97 insertions, 51 deletions

diff --git a/lib/chef/application/windows_service.rb b/lib/chef/application/windows_service.rb
index 2551582c3a..de6d20f79f 100644
--- a/lib/chef/application/windows_service.rb
+++ b/lib/chef/application/windows_service.rb
@@ -45,8 +45,7 @@ class Chef
       option :log_location,
         :short        => "-L LOGLOCATION",
         :long         => "--logfile LOGLOCATION",
-        :description  => "Set the log file location",
-        :default => "#{ENV['SYSTEMDRIVE']}/chef/client.log"
+        :description  => "Set the log file location"
 
       option :splay,
         :short        => "-s SECONDS",
@@ -60,6 +59,8 @@ class Chef
         :description  => "Set the number of seconds to wait between chef-client runs",
         :proc         => lambda { |s| s.to_i }
 
+      DEFAULT_LOG_LOCATION ||= "#{ENV['SYSTEMDRIVE']}/chef/client.log"
+
       def service_init
         @service_action_mutex = Mutex.new
         @service_signal = ConditionVariable.new
@@ -187,8 +188,9 @@ class Chef
           # Pass config params to the new process
           config_params = " --no-fork"
           config_params += " -c #{Chef::Config[:config_file]}" unless  Chef::Config[:config_file].nil?
-          config_params += " -L #{Chef::Config[:log_location]}" unless Chef::Config[:log_location] == STDOUT
+          config_params += " -L #{resolve_log_location}" unless Chef::Config[:log_location] == STDOUT
           # Starts a new process and waits till the process exits
+
           result = shell_out(
             "chef-client #{config_params}",
             :timeout => Chef::Config[:windows_service][:watchdog_timeout],
@@ -235,7 +237,7 @@ class Chef
       # See application.rb for related comments.
 
       def configure_logging
-        Chef::Log.init(MonoLogger.new(Chef::Config[:log_location]))
+        Chef::Log.init(MonoLogger.new(resolve_log_location))
         if want_additional_logger?
           configure_stdout_logger
         end
@@ -264,6 +266,15 @@ class Chef
         Chef::Config[:log_level] == :auto
       end
 
+      # Check if we have a log location and it's not STDOUT
+      def resolve_log_location
+        if Chef::Config[:log_location] && Chef::Config[:log_location].is_a?(String)
+          Chef::Config[:log_location]
+        else
+          DEFAULT_LOG_LOCATION
+        end
+      end
+
       # if log_level is `:auto`, convert it to :warn (when using output formatter)
       # or :info (no output formatter). See also +using_output_formatter?+
       def resolve_log_level
diff --git a/lib/chef/application/windows_service_manager.rb b/lib/chef/application/windows_service_manager.rb
index 44526c1720..5330c1b28b 100644
--- a/lib/chef/application/windows_service_manager.rb
+++ b/lib/chef/application/windows_service_manager.rb
@@ -51,8 +51,7 @@ class Chef
       option :log_location,
         :short        => "-L LOGLOCATION",
         :long         => "--logfile LOGLOCATION",
-        :description  => "Set the log file location for chef-service",
-        :default => "#{ENV['SYSTEMDRIVE']}/chef/client.log"
+        :description  => "Set the log file location for chef-service"
 
       option :help,
         :short        => "-h",
diff --git a/spec/unit/windows_service_spec.rb b/spec/unit/windows_service_spec.rb
index 396584716d..24259a7518 100644
--- a/spec/unit/windows_service_spec.rb
+++ b/spec/unit/windows_service_spec.rb
@@ -21,59 +21,95 @@ if Chef::Platform.windows?
 end
 
 describe "Chef::Application::WindowsService", :windows_only do
-  let (:instance) {Chef::Application::WindowsService.new}
-  let (:shell_out_result) {Object.new}
-  let (:tempfile) {Tempfile.new "log_file"}
+  let(:shell_out_result) { double('shellout', stdout: nil, stderr: nil) }
+  let(:config_options) do
+    {
+      config_file: "test_config_file",
+      log_level: :info
+    }
+  end
+  let(:timeout) { 7200 }
+  let(:shellout_options) do
+    {
+      :timeout => timeout,
+      :logger => Chef::Log
+    }
+  end
+
   before do
-    allow(instance).to receive(:parse_options)
-    allow(shell_out_result).to receive(:stdout)
-    allow(shell_out_result).to receive(:stderr)
+    Chef::Config.merge!(config_options)
+    allow(subject).to receive(:configure_chef)
+    allow(subject).to receive(:parse_options)
+    allow(MonoLogger).to receive(:new)
+    allow(subject).to receive(:running?).and_return(true, false)
+    allow(subject).to receive(:state).and_return(4)
+    subject.service_init
   end
-  it "runs chef-client in new process" do
-    expect(instance).to receive(:configure_chef).twice
-    instance.service_init
-    expect(instance).to receive(:run_chef_client).and_call_original
-    expect(instance).to receive(:shell_out).and_return(shell_out_result)
-    allow(instance).to receive(:running?).and_return(true, false)
-    allow(instance.instance_variable_get(:@service_signal)).to receive(:wait)
-    allow(instance).to receive(:state).and_return(4)
-    instance.service_main
+
+  subject { Chef::Application::WindowsService.new }
+
+  it "passes config params to new process with default options" do
+    expect(subject).to receive(:shell_out).with(
+      "chef-client  --no-fork -c test_config_file -L #{Chef::Application::WindowsService::DEFAULT_LOG_LOCATION}",
+      shellout_options
+    ).and_return(shell_out_result)
+    subject.service_main
   end
 
-  context 'when running chef-client' do
-    it "passes config params to new process with a default timeout of 2 hours (7200 seconds)" do
-      Chef::Config.merge!({:log_location => tempfile.path, :config_file => "test_config_file", :log_level => :info})
-      expect(instance).to receive(:configure_chef).twice
-      instance.service_init
-      allow(instance).to receive(:running?).and_return(true, false)
-      allow(instance.instance_variable_get(:@service_signal)).to receive(:wait)
-      allow(instance).to receive(:state).and_return(4)
-      expect(instance).to receive(:run_chef_client).and_call_original
-      expect(instance).to receive(:shell_out).with("chef-client  --no-fork -c test_config_file -L #{tempfile.path}",
-        {
-          :timeout => 7200,
-          :logger => Chef::Log
-        }).and_return(shell_out_result)
-      instance.service_main
+  context 'has a log location configured' do
+    let(:tempfile) { Tempfile.new 'log_file' }
+    let(:config_options) do
+      {
+        log_location: tempfile.path,
+        config_file: "test_config_file",
+        log_level: :info
+      }
+    end
+
+    after do
       tempfile.unlink
     end
 
-    it "passes config params to new process with a the timeout specified in the config" do
-      Chef::Config.merge!({:log_location => tempfile.path, :config_file => "test_config_file", :log_level => :info})
-      Chef::Config[:windows_service][:watchdog_timeout] = 10
-      expect(instance).to receive(:configure_chef).twice
-      instance.service_init
-      allow(instance).to receive(:running?).and_return(true, false)
-      allow(instance.instance_variable_get(:@service_signal)).to receive(:wait)
-      allow(instance).to receive(:state).and_return(4)
-      expect(instance).to receive(:run_chef_client).and_call_original
-      expect(instance).to receive(:shell_out).with("chef-client  --no-fork -c test_config_file -L #{tempfile.path}",
+    it "uses the configured log location" do
+      expect(subject).to receive(:shell_out).with(
+        "chef-client  --no-fork -c test_config_file -L #{tempfile.path}",
+        shellout_options
+      ).and_return(shell_out_result)
+      subject.service_main
+    end
+
+    context 'configured to STDOUT' do
+      let(:config_options) do
         {
-          :timeout => 10,
-          :logger => Chef::Log
-        }).and_return(shell_out_result)
-      instance.service_main
-      tempfile.unlink
+          log_location: STDOUT,
+          config_file: "test_config_file",
+          log_level: :info
+        }
+      end
+
+      it "does not pass log location to new process" do
+        expect(subject).to receive(:shell_out).with(
+          "chef-client  --no-fork -c test_config_file",
+          shellout_options
+        ).and_return(shell_out_result)
+        subject.service_main
+      end
+    end
+  end
+
+  context 'configueres a watchdog timeout' do
+    let(:timeout) { 10 }
+
+    before do
+      Chef::Config[:windows_service][:watchdog_timeout] = 10
+    end
+
+    it "passes watchdog timeout to new process" do
+      expect(subject).to receive(:shell_out).with(
+        "chef-client  --no-fork -c test_config_file -L #{Chef::Application::WindowsService::DEFAULT_LOG_LOCATION}",
+        shellout_options
+      ).and_return(shell_out_result)
+      subject.service_main
     end
   end
 end