Rework the cleanup logic and always use the santized name + testing improvements

Signed-off-by: Tim Smith <tsmith@chef.io>

2 files changed, 53 insertions, 28 deletions

diff --git a/lib/chef/provider/apt_preference.rb b/lib/chef/provider/apt_preference.rb
index abf8e8c221..669ce8f79e 100644
--- a/lib/chef/provider/apt_preference.rb
+++ b/lib/chef/provider/apt_preference.rb
@@ -48,24 +48,25 @@ class Chef
           action :create
         end
 
-        name = safe_name(new_resource.name)
+        sanitized_prefname = safe_name(new_resource.package_name)
 
-        declare_resource(:file, ::File.join(APT_PREFERENCE_DIR, "#{new_resource.name}.pref")) do
-          action :delete
-          if ::File.exist?(::File.join(APT_PREFERENCE_DIR, "#{new_resource.name}.pref"))
-            Chef::Log.warn "Replacing #{new_resource.name}.pref with #{name}.pref in #{APT_PREFERENCE_DIR}"
+        # cleanup any existing pref files w/o the sanitized name (created by old apt cookbook)
+        if (sanitized_prefname != new_resource.package_name) && ::File.exist?("#{APT_PREFERENCE_DIR}/#{new_resource.package_name}.pref")
+          Chef::Log.warn "Replacing legacy #{new_resource.package_name}.pref with #{sanitized_prefname}.pref in #{APT_PREFERENCE_DIR}"
+          declare_resource(:file, "#{APT_PREFERENCE_DIR}/#{new_resource.package_name}.pref") do
+            action :delete
           end
-          only_if { name != new_resource.name }
         end
 
-        declare_resource(:file, ::File.join(APT_PREFERENCE_DIR, "#{new_resource.name}")) do
-          action :delete
-          if ::File.exist?(::File.join(APT_PREFERENCE_DIR, "#{new_resource.name}"))
-            Chef::Log.warn "Replacing #{new_resource.name} with #{new_resource.name}.pref in #{APT_PREFERENCE_DIR}"
+        # cleanup any existing pref files without the .pref extension (created by old apt cookbook)
+        if ::File.exist?("#{APT_PREFERENCE_DIR}/#{new_resource.package_name}")
+          Chef::Log.warn "Replacing legacy #{new_resource.package_name} with #{sanitized_prefname}.pref in #{APT_PREFERENCE_DIR}"
+          declare_resource(:file, "#{APT_PREFERENCE_DIR}/#{new_resource.package_name}") do
+            action :delete
           end
         end
 
-        declare_resource(:file, ::File.join(APT_PREFERENCE_DIR, "#{name}.pref")) do
+        declare_resource(:file, "#{APT_PREFERENCE_DIR}/#{sanitized_prefname}.pref") do
           mode "0644"
           content preference
           action :create
@@ -73,10 +74,11 @@ class Chef
       end
 
       action :delete do
-        name = safe_name(new_resource.name)
-        if ::File.exist?(::File.join(APT_PREFERENCE_DIR, "#{name}.pref"))
-          Chef::Log.info "Un-pinning #{name} from #{APT_PREFERENCE_DIR}"
-          declare_resource(:file, ::File.join(APT_PREFERENCE_DIR, "#{name}.pref")) do
+        sanitized_prefname = safe_name(new_resource.package_name)
+
+        if ::File.exist?("#{APT_PREFERENCE_DIR}/#{sanitized_prefname}.pref")
+          Chef::Log.info "Un-pinning #{sanitized_prefname} from #{APT_PREFERENCE_DIR}"
+          declare_resource(:file, "#{APT_PREFERENCE_DIR}/#{sanitized_prefname}.pref") do
             action :delete
           end
         end
diff --git a/spec/unit/provider/apt_preference_spec.rb b/spec/unit/provider/apt_preference_spec.rb
index d0dbe8de25..85c10dcbba 100644
--- a/spec/unit/provider/apt_preference_spec.rb
+++ b/spec/unit/provider/apt_preference_spec.rb
@@ -40,24 +40,47 @@ describe Chef::Provider::AptPreference do
     expect(provider).to respond_to(:load_current_resource)
   end
 
-  context "when the preferences.d directory does not exist" do
-    before do
-      FileUtils.rmdir pref_dir
-      expect(File.exist?(pref_dir)).to be false
+  context "#action_add" do
+    context "without a preferences.d directory" do
+      before do
+        FileUtils.rmdir pref_dir
+      end
+
+      it "creates the preferences.d directory" do
+        provider.run_action(:add)
+        expect(new_resource).to be_updated_by_last_action
+        expect(File.exist?(pref_dir)).to be true
+        expect(File.directory?(pref_dir)).to be true
+      end
     end
 
-    it "should create the preferences.d directory" do
-      provider.run_action(:add)
-      expect(new_resource).to be_updated_by_last_action
-      expect(File.exist?(pref_dir)).to be true
-      expect(File.directory?(pref_dir)).to be true
+    context "with a preferences.d directory" do
+      before do
+        FileUtils.mkdir pref_dir unless ::File.exist?(pref_dir)
+        FileUtils.touch("#{pref_dir}/libmysqlclient16.1*.pref")
+        FileUtils.touch("#{pref_dir}/libmysqlclient16.1*")
+      end
+
+      it "creates a sanitized .pref file and removes the legacy cookbook files" do
+        provider.run_action(:add)
+        expect(new_resource).to be_updated_by_last_action
+        expect(File).not_to exist("#{pref_dir}/libmysqlclient16.1*.pref")
+        expect(File).not_to exist("#{pref_dir}/libmysqlclient16.1*")
+        expect(File.read(::File.join(pref_dir, "libmysqlclient16_1wildcard.pref"))).to match(/Package: libmysqlclient16.1*.*Pin: 1.0.1.*Pin-Priority: 1001/m)
+      end
     end
+  end
 
-    it "creates a sanitized .pref file" do
-      provider.run_action(:add)
-      expect(new_resource).to be_updated_by_last_action
-      expect(File.read(::File.join(pref_dir, "libmysqlclient16_1wildcard.pref"))).to match(/Package: libmysqlclient16.1*.*Pin: 1.0.1.*Pin-Priority: 1001/m)
+  context "#action_delete" do
+    before do
+      FileUtils.mkdir pref_dir unless ::File.exist?(pref_dir)
+      FileUtils.touch("#{pref_dir}/libmysqlclient16_1wildcard.pref")
+    end
 
+    it "deletes the name santized .pref file" do
+      provider.run_action(:delete)
+      expect(new_resource).to be_updated_by_last_action
+      expect(File).not_to exist("#{pref_dir}/libmysqlclient16_1wildcard.pref")
     end
   end
 end