Ensure package (un)locking is idempotent

Due to the nature of the comparison done, the `package_locked` methods
will always return false as they presume the package name is a string,
when it is in fact always coerced into an array.

Additionally in situations where the package_name is set rather than
being inherited from the package resource's name, it will always return
false.

Resolves #6361
Resolves #6493

Signed-off-by: Andy Wagner <andy@andywagner.ca>

6 files changed, 75 insertions, 43 deletions

diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb
index 7116bc9045..df3f2a46b1 100644
--- a/lib/chef/provider/package.rb
+++ b/lib/chef/provider/package.rb
@@ -220,7 +220,7 @@ class Chef
       end
 
       def action_lock
-        if package_locked(new_resource.name, new_resource.version) == false
+        unless package_locked(new_resource.package_name, new_resource.version)
           description = new_resource.version ? "version #{new_resource.version} of " : ""
           converge_by("lock #{description}package #{current_resource.package_name}") do
             multipackage_api_adapter(current_resource.package_name, new_resource.version) do |name, version|
@@ -234,7 +234,7 @@ class Chef
       end
 
       def action_unlock
-        if package_locked(new_resource.name, new_resource.version) == true
+        if package_locked(new_resource.package_name, new_resource.version)
           description = new_resource.version ? "version #{new_resource.version} of " : ""
           converge_by("unlock #{description}package #{current_resource.package_name}") do
             multipackage_api_adapter(current_resource.package_name, new_resource.version) do |name, version|
diff --git a/lib/chef/provider/package/apt.rb b/lib/chef/provider/package/apt.rb
index 3f32f9d380..9eb8dd736a 100644
--- a/lib/chef/provider/package/apt.rb
+++ b/lib/chef/provider/package/apt.rb
@@ -71,15 +71,11 @@ class Chef
         end
 
         def package_locked(name, version)
-          islocked = false
           locked = shell_out_compact_timeout!("apt-mark", "showhold")
-          locked.stdout.each_line do |line|
-            line_package = line.strip
-            if line_package == name
-              islocked = true
-            end
+          locked_packages = locked.stdout.each_line.map do |line|
+            line.strip
           end
-          islocked
+          name.all? { |n| locked_packages.include? n }
         end
 
         def install_package(name, version)
diff --git a/lib/chef/provider/package/yum.rb b/lib/chef/provider/package/yum.rb
index 7129104224..4151b88242 100644
--- a/lib/chef/provider/package/yum.rb
+++ b/lib/chef/provider/package/yum.rb
@@ -100,15 +100,11 @@ class Chef
 
         # @see Chef::Provider::Package#package_locked
         def package_locked(name, version)
-          islocked = false
           locked = shell_out_with_timeout!("yum versionlock")
-          locked.stdout.each_line do |line|
-            line_package = line.sub(/-[^-]*-[^-]*$/, "").split(":").last.strip
-            if line_package == name
-              islocked = true
-            end
+          locked_packages = locked.stdout.each_line.map do |line|
+            line.sub(/-[^-]*-[^-]*$/, "").split(":").last.strip
           end
-          islocked
+          name.all? { |n| locked_packages.include? n }
         end
 
         #
diff --git a/lib/chef/provider/package/zypper.rb b/lib/chef/provider/package/zypper.rb
index 77d619d205..05e6dbd8fc 100644
--- a/lib/chef/provider/package/zypper.rb
+++ b/lib/chef/provider/package/zypper.rb
@@ -76,15 +76,11 @@ class Chef
         end
 
         def package_locked(name, version)
-          islocked = false
           locked = shell_out_compact_timeout!("zypper", "locks")
-          locked.stdout.each_line do |line|
-            line_package = line.split("|").shift(2).last.strip
-            if line_package == name
-              islocked = true
-            end
+          locked_packages = locked.stdout.each_line.map do |line|
+            line.split("|").shift(2).last.strip
           end
-          islocked
+          name.all? { |n| locked_packages.include? n }
         end
 
         def load_current_resource
diff --git a/spec/unit/provider/package/apt_spec.rb b/spec/unit/provider/package/apt_spec.rb
index ff14edcffd..6f658fc5bd 100644
--- a/spec/unit/provider/package/apt_spec.rb
+++ b/spec/unit/provider/package/apt_spec.rb
@@ -414,6 +414,17 @@ mpg123 1.12.1-0ubuntu1
             )
             @provider.lock_package("irssi", "0.8.12-7")
           end
+          it "should not lock if the package is already locked" do
+            allow(@provider).to receive(:shell_out_compact_timeout!).with(
+              "apt-mark", "showhold"
+            ).and_return(instance_double(
+              Mixlib::ShellOut, stdout: "irssi")
+            )
+            expect(Chef::Log).to receive(:debug).with("#{@provider.new_resource} is already locked")
+
+            @provider.new_resource.package_name = ["irssi"]
+            @provider.action_lock
+          end
         end
 
         describe "when unlocking a package" do
@@ -425,6 +436,17 @@ mpg123 1.12.1-0ubuntu1
             )
             @provider.unlock_package("irssi", "0.8.12-7")
           end
+          it "should not unlock if the package is already unlocked" do
+            allow(@provider).to receive(:shell_out_compact_timeout!).with(
+              "apt-mark", "showhold"
+            ).and_return(instance_double(
+              Mixlib::ShellOut, stdout: "")
+            )
+            expect(Chef::Log).to receive(:debug).with("#{@provider.new_resource} is already unlocked")
+
+            @provider.new_resource.package_name = ["irssi"]
+            @provider.action_unlock
+          end
         end
 
         describe "when installing a virtual package" do
diff --git a/spec/unit/provider/package_spec.rb b/spec/unit/provider/package_spec.rb
index 122e58efb7..8333d5bd0d 100644
--- a/spec/unit/provider/package_spec.rb
+++ b/spec/unit/provider/package_spec.rb
@@ -27,10 +27,12 @@ describe Chef::Provider::Package do
   end
   let(:events) { Chef::EventDispatch::Dispatcher.new }
   let(:run_context) { Chef::RunContext.new(node, {}, events) }
-  let(:new_resource) { Chef::Resource::Package.new("emacs") }
-  let(:current_resource) { Chef::Resource::Package.new("emacs") }
+  let(:new_resource) { Chef::Resource::Package.new("install emacs") }
+  let(:current_resource) { Chef::Resource::Package.new("install emacs") }
   let(:candidate_version) { "1.0" }
   let(:provider) do
+    new_resource.package_name = "emacs"
+    current_resource.package_name = "emacs"
     provider = Chef::Provider::Package.new(new_resource, run_context)
     provider.current_resource = current_resource
     provider.candidate_version = candidate_version
@@ -57,7 +59,7 @@ describe Chef::Provider::Package do
     it "should call preseed_package if a response_file is given" do
       new_resource.response_file("foo")
       expect(provider).to receive(:get_preseed_file).with(
-        new_resource.name,
+        new_resource.package_name,
         provider.candidate_version
       ).and_return("/var/cache/preseed-test")
 
@@ -74,7 +76,7 @@ describe Chef::Provider::Package do
 
     it "should install the package at the candidate_version if it is not already installed" do
       expect(provider).to receive(:install_package).with(
-        new_resource.name,
+        new_resource.package_name,
         provider.candidate_version
       ).and_return(true)
       provider.run_action(:install)
@@ -84,7 +86,7 @@ describe Chef::Provider::Package do
     it "should install the package at the version specified if it is not already installed" do
       new_resource.version("1.0")
       expect(provider).to receive(:install_package).with(
-        new_resource.name,
+        new_resource.package_name,
         new_resource.version
       ).and_return(true)
       provider.run_action(:install)
@@ -95,7 +97,7 @@ describe Chef::Provider::Package do
       new_resource.version("1.0")
       allow(current_resource).to receive(:version).and_return("0.99")
       expect(provider).to receive(:install_package).with(
-        new_resource.name,
+        new_resource.package_name,
         new_resource.version
       ).and_return(true)
       provider.run_action(:install)
@@ -143,7 +145,7 @@ describe Chef::Provider::Package do
 
     it "should upgrade the package if the current version is not the candidate version" do
       expect(provider).to receive(:upgrade_package).with(
-        new_resource.name,
+        new_resource.package_name,
         provider.candidate_version
       ).and_return(true)
       provider.run_action(:upgrade)
@@ -164,7 +166,7 @@ describe Chef::Provider::Package do
 
     it "should print the word 'uninstalled' if there was no original version" do
       allow(current_resource).to receive(:version).and_return(nil)
-      expect(Chef::Log).to receive(:info).with("package[emacs] upgraded emacs to 1.0")
+      expect(Chef::Log).to receive(:info).with("package[install emacs] upgraded emacs to 1.0")
       provider.run_action(:upgrade)
       expect(new_resource).to be_updated_by_last_action
     end
@@ -285,7 +287,7 @@ describe Chef::Provider::Package do
       expect(provider).to receive(:get_preseed_file).and_return("/var/cache/preseed-test")
       allow(provider).to receive(:preseed_package).and_return(true)
       allow(provider).to receive(:reconfig_package).and_return(true)
-      expect(Chef::Log).to receive(:info).with("package[emacs] reconfigured")
+      expect(Chef::Log).to receive(:info).with("package[install emacs] reconfigured")
       expect(provider).to receive(:reconfig_package)
       provider.run_action(:reconfig)
       expect(new_resource).to be_updated
@@ -294,7 +296,7 @@ describe Chef::Provider::Package do
 
     it "should debug log and not reconfigure the package if the package is not installed" do
       allow(current_resource).to receive(:version).and_return(nil)
-      expect(Chef::Log).to receive(:debug).with("package[emacs] is NOT installed - nothing to do")
+      expect(Chef::Log).to receive(:debug).with("package[install emacs] is NOT installed - nothing to do")
       expect(provider).not_to receive(:reconfig_package)
       provider.run_action(:reconfig)
       expect(new_resource).not_to be_updated_by_last_action
@@ -303,7 +305,7 @@ describe Chef::Provider::Package do
     it "should debug log and not reconfigure the package if no response_file is given" do
       allow(current_resource).to receive(:version).and_return("1.0")
       allow(new_resource).to receive(:response_file).and_return(nil)
-      expect(Chef::Log).to receive(:debug).with("package[emacs] no response_file provided - nothing to do")
+      expect(Chef::Log).to receive(:debug).with("package[install emacs] no response_file provided - nothing to do")
       expect(provider).not_to receive(:reconfig_package)
       provider.run_action(:reconfig)
       expect(new_resource).not_to be_updated_by_last_action
@@ -314,7 +316,7 @@ describe Chef::Provider::Package do
       allow(new_resource).to receive(:response_file).and_return(true)
       expect(provider).to receive(:get_preseed_file).and_return(false)
       allow(provider).to receive(:preseed_package).and_return(false)
-      expect(Chef::Log).to receive(:debug).with("package[emacs] preseeding has not changed - nothing to do")
+      expect(Chef::Log).to receive(:debug).with("package[install emacs] preseeding has not changed - nothing to do")
       expect(provider).not_to receive(:reconfig_package)
       provider.run_action(:reconfig)
       expect(new_resource).not_to be_updated_by_last_action
@@ -323,24 +325,35 @@ describe Chef::Provider::Package do
 
   describe "When locking the package" do
     before(:each) do
-      allow(provider).to receive(:lock_package).and_return(true)
+      allow(provider).to receive(:lock_package).with(
+        new_resource.package_name,
+        nil
+      ).and_return(true)
     end
 
     it "should lock the package if it is unlocked" do
-      allow(provider).to receive(:package_locked).and_return(false)
-      expect(provider).to receive(:lock_package)
+      allow(provider).to receive(:package_locked).with(
+        new_resource.package_name,
+        nil
+      ).and_return(false)
       provider.run_action(:lock)
     end
 
     it "should not lock the package if it is already locked" do
-      allow(provider).to receive(:package_locked).and_return(true)
+      allow(provider).to receive(:package_locked).with(
+        new_resource.package_name,
+        nil
+      ).and_return(true)
       expect(provider).not_to receive(:lock_package)
       provider.run_action(:lock)
       expect(new_resource).not_to be_updated_by_last_action
     end
 
     it "should set the resource to updated if it locks the package" do
-      allow(provider).to receive(:package_locked).and_return(false)
+      allow(provider).to receive(:package_locked).with(
+        new_resource.package_name,
+        nil
+      ).and_return(false)
       provider.run_action(:lock)
       expect(new_resource).to be_updated
     end
@@ -352,20 +365,29 @@ describe Chef::Provider::Package do
     end
 
     it "should unlock the package if it is locked" do
-      allow(provider).to receive(:package_locked).and_return(true)
+      allow(provider).to receive(:package_locked).with(
+        new_resource.package_name,
+        nil
+      ).and_return(true)
       expect(provider).to receive(:unlock_package)
       provider.run_action(:unlock)
     end
 
     it "should not unlock the package if it is already unlocked" do
-      allow(provider).to receive(:package_locked).and_return(false)
+      allow(provider).to receive(:package_locked).with(
+        new_resource.package_name,
+        nil
+      ).and_return(false)
       expect(provider).not_to receive(:unlock_package)
       provider.run_action(:unlock)
       expect(new_resource).not_to be_updated_by_last_action
     end
 
     it "should set the resource to updated if it unlocks the package" do
-      allow(provider).to receive(:package_locked).and_return(true)
+      allow(provider).to receive(:package_locked).with(
+        new_resource.package_name,
+        nil
+      ).and_return(true)
       provider.run_action(:unlock)
       expect(new_resource).to be_updated
     end