dpkg provider cleanup

- :update and :install are now treated the same way and throw the
  same exceptions
- :remove and :purge don't require the source at all, so don't do
  any checking on that
- fix some convoluted side-effecty logic in load_current_resource
- load_current_resource now correctly gets the dpkg state on
  :remove and :purge when the file does not exist (pretty sure
  the old logic did not)
- fixed the FIXME about using en_US.UTF-8 (the default for shell_out!)
- just use shell_out! to throw exceptions
- clean up all the specs and remove all the instance vars from the code

2 files changed, 219 insertions, 145 deletions

diff --git a/lib/chef/provider/package/dpkg.rb b/lib/chef/provider/package/dpkg.rb
index 67e9b903c6..2de6226bb9 100644
--- a/lib/chef/provider/package/dpkg.rb
+++ b/lib/chef/provider/package/dpkg.rb
@@ -34,83 +34,61 @@ class Chef
 
         def define_resource_requirements
           super
-          requirements.assert(:install) do |a|
-            a.assertion{ not @new_resource.source.nil? }
-            a.failure_message Chef::Exceptions::Package, "Source for package #{@new_resource.name} required for action install"
+
+          requirements.assert(:install, :upgrade) do |a|
+            a.assertion { !new_resource.source.nil? }
+            a.failure_message Chef::Exceptions::Package, "#{new_resource} the source property is required for action :install or :upgrade"
           end
 
-          # TODO this was originally written for any action in which .source is provided
-          # but would it make more sense to only look at source if the action is :install?
-          requirements.assert(:all_actions) do |a|
-            a.assertion { @source_exists }
-            a.failure_message Chef::Exceptions::Package, "Package #{@new_resource.name} not found: #{@new_resource.source}"
-            a.whyrun "Assuming it would have been previously downloaded."
+          requirements.assert(:install, :upgrade) do |a|
+            a.assertion { source_file_exist? }
+            a.failure_message Chef::Exceptions::Package, "#{new_resource} source file does not exist: #{new_resource.source}"
+            a.whyrun "Assuming it would have been previously created."
           end
         end
 
         def load_current_resource
-          @source_exists = true
-          @current_resource = Chef::Resource::Package.new(@new_resource.name)
-          @current_resource.package_name(@new_resource.package_name)
-
-          if @new_resource.source
-            @source_exists = ::File.exists?(@new_resource.source)
-            if @source_exists
-              # Get information from the package if supplied
-              Chef::Log.debug("#{@new_resource} checking dpkg status")
-              status = shell_out_with_timeout("dpkg-deb -W #{@new_resource.source}")
-              pkginfo = status.stdout.split("\t")
-              unless pkginfo.empty?
-                @current_resource.package_name(pkginfo[0])
-                @candidate_version = pkginfo[1].strip
-              end
-            else
-              # Source provided but not valid means we can't safely do further processing
-              return
-            end
+          @current_resource = Chef::Resource::Package.new(new_resource.name)
+          current_resource.package_name(new_resource.package_name)
+
+          if source_file_exist?
+            @candidate_version = get_candidate_version
+            current_resource.package_name(get_package_name)
+            # if the source file exists then our package_name is right
+            current_resource.version(get_current_version)
+          elsif !installing?
+            # we can't do this if we're installing with no source, because our package_name
+            # is probably not right.
+            #
+            # if we're removing or purging we don't use source, and our package_name must
+            # be right so we can do this.
+            #
+            # we don't error here on the dpkg command since we'll handle the exception or
+            # the why-run message in define_resource_requirements.
+            current_resource.version(get_current_version)
           end
 
-          # Check to see if it is installed
-          package_installed = nil
-          Chef::Log.debug("#{@new_resource} checking install state")
-          status = shell_out_with_timeout("dpkg -s #{@current_resource.package_name}")
-          status.stdout.each_line do |line|
-            case line
-            when DPKG_INSTALLED
-              package_installed = true
-            when DPKG_VERSION
-              if package_installed
-                Chef::Log.debug("#{@new_resource} current version is #{$1}")
-                @current_resource.version($1)
-              end
-            end
-          end
-
-          unless status.exitstatus == 0 || status.exitstatus == 1
-            raise Chef::Exceptions::Package, "dpkg failed - #{status.inspect}!"
-          end
-
-          @current_resource
+          current_resource
         end
 
         def install_package(name, version)
-          Chef::Log.info("#{@new_resource} installing #{@new_resource.source}")
+          Chef::Log.info("#{new_resource} installing #{new_resource.source}")
           run_noninteractive(
-            "dpkg -i#{expand_options(@new_resource.options)} #{@new_resource.source}"
+            "dpkg -i#{expand_options(new_resource.options)} #{new_resource.source}"
           )
         end
 
         def remove_package(name, version)
-          Chef::Log.info("#{@new_resource} removing #{@new_resource.package_name}")
+          Chef::Log.info("#{new_resource} removing #{new_resource.package_name}")
           run_noninteractive(
-            "dpkg -r#{expand_options(@new_resource.options)} #{@new_resource.package_name}"
+            "dpkg -r#{expand_options(new_resource.options)} #{new_resource.package_name}"
           )
         end
 
         def purge_package(name, version)
-          Chef::Log.info("#{@new_resource} purging #{@new_resource.package_name}")
+          Chef::Log.info("#{new_resource} purging #{new_resource.package_name}")
           run_noninteractive(
-            "dpkg -P#{expand_options(@new_resource.options)} #{@new_resource.package_name}"
+            "dpkg -P#{expand_options(new_resource.options)} #{new_resource.package_name}"
           )
         end
 
@@ -119,22 +97,65 @@ class Chef
         end
 
         def preseed_package(preseed_file)
-          Chef::Log.info("#{@new_resource} pre-seeding package installation instructions")
+          Chef::Log.info("#{new_resource} pre-seeding package installation instructions")
           run_noninteractive("debconf-set-selections #{preseed_file}")
         end
 
         def reconfig_package(name, version)
-          Chef::Log.info("#{@new_resource} reconfiguring")
+          Chef::Log.info("#{new_resource} reconfiguring")
           run_noninteractive("dpkg-reconfigure #{name}")
         end
 
+        private
+
+        def get_current_version
+          Chef::Log.debug("#{new_resource} checking install state")
+          status = shell_out_with_timeout("dpkg -s #{current_resource.package_name}")
+          package_installed = false
+          status.stdout.each_line do |line|
+            case line
+            when DPKG_INSTALLED
+              package_installed = true
+            when DPKG_VERSION
+              if package_installed
+                Chef::Log.debug("#{new_resource} current version is #{$1}")
+                return $1
+              end
+            end
+          end
+          return nil
+        end
+
         # Runs command via shell_out_with_timeout with magic environment to disable
         # interactive prompts. Command is run with default localization rather
         # than forcing locale to "C", so command output may not be stable.
-        #
-        # FIXME: This should be "LC_ALL" => "en_US.UTF-8" in order to stabilize the output and get UTF-8
         def run_noninteractive(command)
-          shell_out_with_timeout!(command, :env => { "DEBIAN_FRONTEND" => "noninteractive", "LC_ALL" => nil })
+          shell_out_with_timeout!(command, :env => { "DEBIAN_FRONTEND" => "noninteractive" })
+        end
+
+        def source_file_exist?
+          new_resource.source && ::File.exist?(new_resource.source)
+        end
+
+        def pkginfo
+          @pkginfo ||=
+            begin
+              Chef::Log.debug("#{new_resource} checking dpkg status")
+              status = shell_out_with_timeout!("dpkg-deb -W #{new_resource.source}")
+              status.stdout.split("\t")
+            end
+        end
+
+        def get_candidate_version
+          pkginfo[1].strip unless pkginfo.empty?
+        end
+
+        def get_package_name
+          pkginfo[0] unless pkginfo.empty?
+        end
+
+        def installing?
+          [:install, :upgrade].include?(action)
         end
 
       end
diff --git a/spec/unit/provider/package/dpkg_spec.rb b/spec/unit/provider/package/dpkg_spec.rb
index b868128147..2ded8313c0 100644
--- a/spec/unit/provider/package/dpkg_spec.rb
+++ b/spec/unit/provider/package/dpkg_spec.rb
@@ -19,42 +19,109 @@
 require 'spec_helper'
 
 describe Chef::Provider::Package::Dpkg do
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:package) { "wget" }
+  let(:source) { "/tmp/wget_1.11.4-1ubuntu1_amd64.deb" }
+  let(:new_resource) do
+    new_resource = Chef::Resource::Package.new(package)
+    new_resource.source source
+    new_resource
+  end
+  let(:provider) { Chef::Provider::Package::Dpkg.new(new_resource, run_context) }
+
+  let(:dpkg_deb_version) { "1.11.4" }
+  let(:dpkg_deb_status) { status = double(:stdout => "#{package}\t#{dpkg_deb_version}", :exitstatus => 0) }
+  let(:dpkg_s_version) { "1.11.4-1ubuntu1" }
+  let(:dpkg_s_status) do
+    stdout = <<-DPKG_S
+Package: #{package}
+Status: install ok installed
+Priority: important
+Section: web
+Installed-Size: 1944
+Maintainer: Ubuntu Core developers <ubuntu-devel-discuss@lists.ubuntu.com>
+Architecture: amd64
+Version: #{dpkg_s_version}
+Config-Version: #{dpkg_s_version}
+Depends: libc6 (>= 2.8~20080505), libssl0.9.8 (>= 0.9.8f-5)
+Conflicts: wget-ssl
+    DPKG_S
+    status = double(:stdout => stdout, :exitstatus => 1)
+  end
+
   before(:each) do
-    @node = Chef::Node.new
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, {}, @events)
-    @new_resource = Chef::Resource::Package.new("wget")
-    @new_resource.source "/tmp/wget_1.11.4-1ubuntu1_amd64.deb"
+    allow(provider).to receive(:shell_out!).with("dpkg-deb -W #{new_resource.source}", timeout: 900).and_return(dpkg_deb_status)
+    allow(provider).to receive(:shell_out).with("dpkg -s #{package}", timeout: 900).and_return(dpkg_s_status)
+    allow(::File).to receive(:exist?).with(source).and_return(true)
+  end
+
+  describe "#define_resource_requirements" do
+    it "should raise an exception if a source is supplied but not found when :install" do
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect { provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package)
+    end
+
+    it "should raise an exception if a source is supplied but not found when :upgrade" do
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect { provider.run_action(:upgrade) }.to raise_error(Chef::Exceptions::Package)
+    end
+
+    # FIXME?  we're saying we ignore source, but should supplying source on :remove or :purge be an actual error?
+    it "should not raise an exception if a source is supplied but not found when :remove" do
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect(provider).to receive(:action_remove)
+      expect { provider.run_action(:remove) }.not_to raise_error
+    end
+
+    it "should not raise an exception if a source is supplied but not found when :purge" do
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect(provider).to receive(:action_purge)
+      expect { provider.run_action(:purge) }.not_to raise_error
+    end
 
-    @provider = Chef::Provider::Package::Dpkg.new(@new_resource, @run_context)
+    it "should raise an exception if a source is nil when :install" do
+      new_resource.source nil
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect { provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package)
+    end
 
-    @status = double(:stdout => "", :exitstatus => 0)
-    allow(@provider).to receive(:shell_out).and_return(@status)
+    it "should raise an exception if a source is nil when :upgrade" do
+      new_resource.source nil
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect { provider.run_action(:upgrade) }.to raise_error(Chef::Exceptions::Package)
+    end
 
-    allow(::File).to receive(:exists?).and_return(true)
+    it "should not raise an exception if a source is nil when :remove" do
+      new_resource.source nil
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect(provider).to receive(:action_remove)
+      expect { provider.run_action(:remove) }.not_to raise_error
+    end
+
+    it "should not raise an exception if a source is nil when :purge" do
+      new_resource.source nil
+      allow(::File).to receive(:exist?).with(source).and_return(false)
+      expect(provider).to receive(:action_purge)
+      expect { provider.run_action(:purge) }.not_to raise_error
+    end
   end
 
   describe "when loading the current resource state" do
 
     it "should create a current resource with the name of the new_resource" do
-      @provider.load_current_resource
-      expect(@provider.current_resource.package_name).to eq("wget")
-    end
-
-    it "should raise an exception if a source is supplied but not found" do
-      @provider.load_current_resource
-      @provider.define_resource_requirements
-      allow(::File).to receive(:exists?).and_return(false)
-      expect { @provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package)
+      provider.load_current_resource
+      expect(provider.current_resource.package_name).to eq("wget")
     end
 
     describe 'gets the source package version from dpkg-deb' do
       def check_version(version)
-        @status = double(:stdout => "wget\t#{version}", :exitstatus => 0)
-        allow(@provider).to receive(:shell_out).with("dpkg-deb -W #{@new_resource.source}", timeout: 900).and_return(@status)
-        @provider.load_current_resource
-        expect(@provider.current_resource.package_name).to eq("wget")
-        expect(@provider.candidate_version).to eq(version)
+        status = double(:stdout => "wget\t#{version}", :exitstatus => 0)
+        expect(provider).to receive(:shell_out!).with("dpkg-deb -W #{new_resource.source}", timeout: 900).and_return(status)
+        provider.load_current_resource
+        expect(provider.current_resource.package_name).to eq("wget")
+        expect(provider.candidate_version).to eq(version)
       end
 
       it 'if short version provided' do
@@ -74,129 +141,115 @@ describe Chef::Provider::Package::Dpkg do
       end
     end
 
-    it "gets the source package name from dpkg-deb correctly when the package name has `-', `+' or `.' characters" do
-      stdout = "f.o.o-pkg++2\t1.11.4-1ubuntu1"
-      status = double(:stdout => stdout, :exitstatus => 1)
-      allow(@provider).to receive(:shell_out).and_return(status)
-      @provider.load_current_resource
-      expect(@provider.current_resource.package_name).to eq("f.o.o-pkg++2")
+    describe "when the package name has `-', `+' or `.' characters" do
+      let(:package) { "f.o.o-pkg++2" }
+
+      it "gets the source package name from dpkg-deb correctly" do
+        provider.load_current_resource
+        expect(provider.current_resource.package_name).to eq("f.o.o-pkg++2")
+      end
     end
 
-    it "gets the source package version from dpkg-deb correctly when the package version has `~', `-', `+' or `.' characters" do
-      stdout = "b.a.r-pkg++1\t1.2.3+3141592-1ubuntu1~lucid"
-      status = double(:stdout => stdout, :exitstatus => 1)
-      allow(@provider).to receive(:shell_out).and_return(status)
-      @provider.load_current_resource
-      expect(@provider.candidate_version).to eq('1.2.3+3141592-1ubuntu1~lucid')
+    describe "when the package version has `~', `-', `+' or `.' characters" do
+      let(:package) { "b.a.r-pkg++1" }
+      let(:dpkg_deb_version) { "1.2.3+3141592-1ubuntu1~lucid" }
+      let(:dpkg_s_version) { "1.2.3+3141592-1ubuntu1~lucid" }
+
+      it "gets the source package version from dpkg-deb correctly when the package version has `~', `-', `+' or `.' characters" do
+        provider.load_current_resource
+        expect(provider.candidate_version).to eq('1.2.3+3141592-1ubuntu1~lucid')
+      end
     end
 
     it "should raise an exception if the source is not set but we are installing" do
-      @new_resource = Chef::Resource::Package.new("wget")
-      @provider.new_resource = @new_resource
-      @provider.load_current_resource
-      @provider.define_resource_requirements
-      expect { @provider.run_action(:install)}.to raise_error(Chef::Exceptions::Package)
+      new_resource = Chef::Resource::Package.new("wget")
+      provider.new_resource = new_resource
+      provider.load_current_resource
+      provider.define_resource_requirements
+      expect { provider.run_action(:install)}.to raise_error(Chef::Exceptions::Package)
     end
 
     it "should return the current version installed if found by dpkg" do
-      stdout = <<-DPKG_S
-Package: wget
-Status: install ok installed
-Priority: important
-Section: web
-Installed-Size: 1944
-Maintainer: Ubuntu Core developers <ubuntu-devel-discuss@lists.ubuntu.com>
-Architecture: amd64
-Version: 1.11.4-1ubuntu1
-Config-Version: 1.11.4-1ubuntu1
-Depends: libc6 (>= 2.8~20080505), libssl0.9.8 (>= 0.9.8f-5)
-Conflicts: wget-ssl
-DPKG_S
-      status = double(:stdout => stdout, :exitstatus => 1)
-      allow(@provider).to receive(:shell_out).with("dpkg -s wget", timeout: 900).and_return(status)
-
-      @provider.load_current_resource
-      expect(@provider.current_resource.version).to eq("1.11.4-1ubuntu1")
+      provider.load_current_resource
+      expect(provider.current_resource.version).to eq("1.11.4-1ubuntu1")
     end
 
     it "should raise an exception if dpkg fails to run" do
       status = double(:stdout => "", :exitstatus => -1)
-      allow(@provider).to receive(:shell_out).and_return(status)
-      expect { @provider.load_current_resource }.to raise_error(Chef::Exceptions::Package)
+      expect(provider).to receive(:shell_out_with_timeout!).with("dpkg-deb -W /tmp/wget_1.11.4-1ubuntu1_amd64.deb").and_raise(Mixlib::ShellOut::ShellCommandFailed)
+      expect { provider.load_current_resource }.to raise_error(Mixlib::ShellOut::ShellCommandFailed)
     end
   end
 
   describe Chef::Provider::Package::Dpkg, "install and upgrade" do
     it "should run dpkg -i with the package source" do
-      expect(@provider).to receive(:run_noninteractive).with(
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -i /tmp/wget_1.11.4-1ubuntu1_amd64.deb"
       )
-      @provider.install_package("wget", "1.11.4-1ubuntu1")
+      provider.install_package("wget", "1.11.4-1ubuntu1")
     end
 
     it "should run dpkg -i if the package is a path and the source is nil" do
-      @new_resource = Chef::Resource::Package.new("/tmp/wget_1.11.4-1ubuntu1_amd64.deb")
-      @provider = Chef::Provider::Package::Dpkg.new(@new_resource, @run_context)
-      expect(@provider).to receive(:run_noninteractive).with(
+      new_resource.name = "/tmp/wget_1.11.4-1ubuntu1_amd64.deb"
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -i /tmp/wget_1.11.4-1ubuntu1_amd64.deb"
       )
-      @provider.install_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1")
+      provider.install_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1")
     end
 
     it "should run dpkg -i if the package is a path and the source is nil for an upgrade" do
-      @new_resource = Chef::Resource::Package.new("/tmp/wget_1.11.4-1ubuntu1_amd64.deb")
-      @provider = Chef::Provider::Package::Dpkg.new(@new_resource, @run_context)
-      expect(@provider).to receive(:run_noninteractive).with(
+      new_resource.name = "/tmp/wget_1.11.4-1ubuntu1_amd64.deb"
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -i /tmp/wget_1.11.4-1ubuntu1_amd64.deb"
       )
-      @provider.upgrade_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1")
+      provider.upgrade_package("/tmp/wget_1.11.4-1ubuntu1_amd64.deb", "1.11.4-1ubuntu1")
     end
 
     it "should run dpkg -i with the package source and options if specified" do
-      expect(@provider).to receive(:run_noninteractive).with(
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -i --force-yes /tmp/wget_1.11.4-1ubuntu1_amd64.deb"
       )
-      allow(@new_resource).to receive(:options).and_return("--force-yes")
+      allow(new_resource).to receive(:options).and_return("--force-yes")
 
-      @provider.install_package("wget", "1.11.4-1ubuntu1")
+      provider.install_package("wget", "1.11.4-1ubuntu1")
     end
     it "should upgrade by running install_package" do
-      expect(@provider).to receive(:install_package).with("wget", "1.11.4-1ubuntu1")
-      @provider.upgrade_package("wget", "1.11.4-1ubuntu1")
+      expect(provider).to receive(:install_package).with("wget", "1.11.4-1ubuntu1")
+      provider.upgrade_package("wget", "1.11.4-1ubuntu1")
     end
   end
 
   describe Chef::Provider::Package::Dpkg, "remove and purge" do
     it "should run dpkg -r to remove the package" do
-      expect(@provider).to receive(:run_noninteractive).with(
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -r wget"
       )
-      @provider.remove_package("wget", "1.11.4-1ubuntu1")
+      provider.remove_package("wget", "1.11.4-1ubuntu1")
     end
 
     it "should run dpkg -r to remove the package with options if specified" do
-      expect(@provider).to receive(:run_noninteractive).with(
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -r --force-yes wget"
       )
-      allow(@new_resource).to receive(:options).and_return("--force-yes")
+      allow(new_resource).to receive(:options).and_return("--force-yes")
 
-      @provider.remove_package("wget", "1.11.4-1ubuntu1")
+      provider.remove_package("wget", "1.11.4-1ubuntu1")
     end
 
     it "should run dpkg -P to purge the package" do
-      expect(@provider).to receive(:run_noninteractive).with(
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -P wget"
       )
-      @provider.purge_package("wget", "1.11.4-1ubuntu1")
+      provider.purge_package("wget", "1.11.4-1ubuntu1")
     end
 
     it "should run dpkg -P to purge the package with options if specified" do
-      expect(@provider).to receive(:run_noninteractive).with(
+      expect(provider).to receive(:run_noninteractive).with(
         "dpkg -P --force-yes wget"
       )
-      allow(@new_resource).to receive(:options).and_return("--force-yes")
+      allow(new_resource).to receive(:options).and_return("--force-yes")
 
-      @provider.purge_package("wget", "1.11.4-1ubuntu1")
+      provider.purge_package("wget", "1.11.4-1ubuntu1")
     end
   end
 end