Merge pull request #9868 from chef/lcg/chef-16-launchd2

4 files changed, 252 insertions, 51 deletions

diff --git a/lib/chef/provider/launchd.rb b/lib/chef/provider/launchd.rb
index 579a53fe03..78ae823596 100644
--- a/lib/chef/provider/launchd.rb
+++ b/lib/chef/provider/launchd.rb
@@ -62,23 +62,23 @@ class Chef
       end
 
       action :delete do
-        # If you delete a service you want to make sure its not loaded or
-        # the service will be in memory and you wont be able to stop it.
-        if ::File.exists?(@path)
+        if ::File.exists?(path)
           manage_service(:disable)
         end
         manage_plist(:delete)
       end
 
       action :enable do
-        if manage_plist(:create)
-          manage_service(:restart)
-        else
-          manage_service(:enable)
+        manage_service(:nothing)
+        manage_plist(:create) do
+          notifies :restart, "macosx_service[#{label}]", :immediately
         end
+        manage_service(:enable)
       end
 
       action :disable do
+        return unless ::File.exist?(path)
+
         manage_service(:disable)
       end
 
@@ -86,13 +86,14 @@ class Chef
         manage_service(:restart)
       end
 
-      def manage_plist(action)
+      def manage_plist(action, &block)
         if source
           cookbook_file path do
             cookbook_name = new_resource.cookbook if new_resource.cookbook
             copy_properties_from(new_resource, :backup, :group, :mode, :owner, :source)
             action(action)
             only_if { manage_agent?(action) }
+            instance_eval(&block) if block_given?
           end
         else
           file path do
@@ -100,6 +101,7 @@ class Chef
             content(file_content) if file_content?
             action(action)
             only_if { manage_agent?(action) }
+            instance_eval(&block) if block_given?
           end
         end
       end
@@ -207,7 +209,7 @@ class Chef
 
       # @api private
       def path
-        @path = new_resource.path ? new_resource.path : gen_path_from_type
+        @path ||= new_resource.path ? new_resource.path : gen_path_from_type
       end
     end
   end
diff --git a/lib/chef/provider/service/macosx.rb b/lib/chef/provider/service/macosx.rb
index ecd0e9e455..a2f8f15b7b 100644
--- a/lib/chef/provider/service/macosx.rb
+++ b/lib/chef/provider/service/macosx.rb
@@ -142,6 +142,15 @@ class Chef
         #
         # This makes some sense on macOS since launchctl is an "init"-style
         # supervisor that will restart daemons that are crashing, etc.
+        #
+        # FIXME: Does this make any sense at all?  The difference between enabled and
+        # running as state would seem to only be useful for completely broken
+        # services (enabled, not restarting, but not running => totally broken?).
+        #
+        # It seems like otherwise :enable is equivalent to :start, and :disable is
+        # equivalent to :stop?  But just with strangely different behavior in the
+        # face of a broken service?
+        #
         def enable_service
           if @current_resource.enabled
             logger.trace("#{@new_resource} already enabled, not enabling")
diff --git a/spec/functional/resource/launchd_spec.rb b/spec/functional/resource/launchd_spec.rb
new file mode 100644
index 0000000000..70399d310d
--- /dev/null
+++ b/spec/functional/resource/launchd_spec.rb
@@ -0,0 +1,232 @@
+#
+# Copyright:: Copyright (c) Chef Software 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 "spec_helper"
+
+# Deploy relies heavily on symlinks, so it doesn't work on windows.
+describe Chef::Resource::Launchd, :macos_only, requires_root: true do
+  include RecipeDSLHelper
+
+  before(:each) do
+    shell_out("launchctl unload -wF /Library/LaunchDaemons/io.chef.testing.fake.plist")
+    FileUtils.rm_f "/Library/LaunchDaemons/io.chef.testing.fake.plist"
+  end
+
+  after(:each) do
+    shell_out("launchctl unload -wF /Library/LaunchDaemons/io.chef.testing.fake.plist")
+    FileUtils.rm_f "/Library/LaunchDaemons/io.chef.testing.fake.plist"
+  end
+
+  context ":enable" do
+    it "enables a service" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :enable
+      end.should_be_updated
+      expect(File.exist?("/Library/LaunchDaemons/io.chef.testing.fake.plist")).to be true
+      expect(shell_out!("launchctl list io.chef.testing.fake").stdout).to match('"PID" = \d+')
+      expect(shell_out!("launchctl list io.chef.testing.fake").stdout).not_to match('"PID" = 0')
+    end
+
+    it "should be idempotent" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :enable
+      end.should_be_updated
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :enable
+      end.should_not_be_updated
+    end
+  end
+
+  context ":create" do
+    it "creates a service" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :create
+      end.should_be_updated
+      expect(File.exist?("/Library/LaunchDaemons/io.chef.testing.fake.plist")).to be true
+      expect(shell_out("launchctl list io.chef.testing.fake").exitstatus).not_to eql(0)
+    end
+
+    it "should be idempotent" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :create
+      end.should_be_updated
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :create
+      end.should_not_be_updated
+    end
+  end
+
+  context ":create_if_missing" do
+    it "creates a service if it is missing" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :create_if_missing
+      end.should_be_updated
+      expect(File.exist?("/Library/LaunchDaemons/io.chef.testing.fake.plist")).to be true
+      expect(shell_out("launchctl list io.chef.testing.fake").exitstatus).not_to eql(0)
+    end
+    it "is idempotent" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :create_if_missing
+      end.should_be_updated
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :create_if_missing
+      end.should_not_be_updated
+    end
+  end
+
+  context ":delete" do
+    it "deletes a service" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :enable
+      end
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :delete
+      end.should_be_updated
+      expect(File.exist?("/Library/LaunchDaemons/io.chef.testing.fake.plist")).to be false
+      expect(shell_out("launchctl list io.chef.testing.fake").exitstatus).not_to eql(0)
+    end
+    it "is idempotent" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "60",
+        ]
+        run_at_load true
+        type "daemon"
+        action :enable
+      end
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :delete
+      end.should_be_updated
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :delete
+      end.should_not_be_updated
+    end
+    it "works if the file does not exist" do
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :delete
+      end.should_not_be_updated
+    end
+  end
+
+  context ":disable" do
+    it "deletes a service" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "1",
+        ]
+        type "daemon"
+        action :enable
+      end
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :disable
+      end.should_be_updated
+      expect(File.exist?("/Library/LaunchDaemons/io.chef.testing.fake.plist")).to be true
+      expect(shell_out("launchctl list io.chef.testing.fake").exitstatus).not_to eql(0)
+    end
+    it "is idempotent" do
+      launchd "io.chef.testing.fake" do
+        program_arguments [
+          "/bin/sleep",
+          "1",
+        ]
+        type "daemon"
+        action :enable
+      end
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :disable
+      end.should_be_updated
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :disable
+      end.should_not_be_updated
+    end
+    it "should work if the plist does not exist" do
+      launchd "io.chef.testing.fake" do
+        type "daemon"
+        action :disable
+      end.should_not_be_updated
+    end
+  end
+end
diff --git a/spec/unit/provider/launchd_spec.rb b/spec/unit/provider/launchd_spec.rb
index 69f0f315d0..6a729c2cb6 100644
--- a/spec/unit/provider/launchd_spec.rb
+++ b/spec/unit/provider/launchd_spec.rb
@@ -194,48 +194,6 @@ describe Chef::Provider::Launchd do
       end
     end
 
-    describe "with an :enable action" do
-      describe "and the file has been updated" do
-        before(:each) do
-          allow(provider).to receive(
-            :manage_plist
-          ).with(:create).and_return(true)
-          allow(provider).to receive(
-            :manage_service
-          ).with(:restart).and_return(true)
-        end
-
-        it "should call manage_service with a :restart action" do
-          expect(provider.manage_service(:restart)).to be_truthy
-        end
-
-        it "works with action enable" do
-          expect(run_resource_setup_for_action(:enable)).to be_truthy
-          provider.action_enable
-        end
-      end
-
-      describe "and the file has not been updated" do
-        before(:each) do
-          allow(provider).to receive(
-            :manage_plist
-          ).with(:create).and_return(nil)
-          allow(provider).to receive(
-            :manage_service
-          ).with(:enable).and_return(true)
-        end
-
-        it "should call manage_service with a :enable action" do
-          expect(provider.manage_service(:enable)).to be_truthy
-        end
-
-        it "works with action enable" do
-          expect(run_resource_setup_for_action(:enable)).to be_truthy
-          provider.action_enable
-        end
-      end
-    end
-
     describe "with an :delete action" do
       describe "and the ld file is present" do
         before(:each) do