lazier assembly of data, more multipackaging

2 files changed, 52 insertions, 51 deletions

diff --git a/lib/chef/provider/package/apt.rb b/lib/chef/provider/package/apt.rb
index 91c58a79ad..6bf94eb1a4 100644
--- a/lib/chef/provider/package/apt.rb
+++ b/lib/chef/provider/package/apt.rb
@@ -28,18 +28,14 @@ class Chef
         provides :package, platform_family: "debian"
         provides :apt_package, os: "linux"
 
-        # return [Hash] mapping of package name to Boolean value
-        attr_accessor :is_virtual_package
-
         def initialize(new_resource, run_context)
           super
-          @is_virtual_package = {}
         end
 
         def load_current_resource
           @current_resource = Chef::Resource::AptPackage.new(new_resource.name)
           current_resource.package_name(new_resource.package_name)
-          check_all_packages_state(new_resource.package_name)
+          current_resource.version(get_current_versions)
           current_resource
         end
 
@@ -60,26 +56,26 @@ class Chef
         # FIXME: need spec to check that candidate_version is set correctly on a virtual package
         # FIXME: need spec to check that packages missing a candidate_version can be removed/purged
 
-        def get_package_versions(pkg)
-          installed_version  = nil
-          candidate_version  = nil
+        def resolve_package_versions(pkg)
+          current_version = nil
+          candidate_version = nil
           run_noninteractive("apt-cache", default_release_options, "policy", pkg).stdout.each_line do |line|
             case line
             when /^\s{2}Installed: (.+)$/
-              installed_version = ( $1 != "(none)" ) ? $1 : nil
+              current_version = ( $1 != "(none)" ) ? $1 : nil
               Chef::Log.debug("#{new_resource} installed version for #{pkg} is #{$1}")
             when /^\s{2}Candidate: (.+)$/
               candidate_version = ( $1 != "(none)" ) ? $1 : nil
               Chef::Log.debug("#{new_resource} candidate version for #{pkg} is #{$1}")
             end
           end
-          [ installed_version, candidate_version ]
+          [ current_version, candidate_version ]
         end
 
         def resolve_virtual_package_name(pkg)
           showpkg = run_noninteractive("apt-cache showpkg", pkg).stdout
           partitions = showpkg.rpartition(/Reverse Provides: ?#{$/}/)
-          return nil if partitions[0] == "" && partitions[1] == ""  # not found in output
+          return nil if partitions[0] == "" && partitions[1] == "" # not found in output
           set = partitions[2].lines.each_with_object(Set.new) do |line, acc|
             # there may be multiple reverse provides for a single package
             acc.add(line.split[0])
@@ -90,59 +86,55 @@ class Chef
           return set.to_a.first
         end
 
-        def check_package_state(pkg)
-          is_virtual_package = false
-          installed_version  = nil
-          candidate_version  = nil
-
+        def package_data_for(pkg)
+          virtual           = false
+          current_version   = nil
+          candidate_version = nil
 
-          installed_version, candidate_version = get_package_versions(pkg)
+          current_version, candidate_version = resolve_package_versions(pkg)
 
           if candidate_version.nil?
             newpkg = resolve_virtual_package_name(pkg)
 
             if newpkg
-              is_virtual_package = true
+              virtual = true
               Chef::Log.info("#{new_resource} is a virtual package, actually acting on package[#{newpkg}]")
-              installed_version, candidate_version = get_package_versions(newpkg)
+              current_version, candidate_version = resolve_package_versions(newpkg)
             end
           end
 
           return {
-            installed_version:   installed_version,
-            candidate_version:   candidate_version,
-            is_virtual_package:  is_virtual_package,
+            current_version:    current_version,
+            candidate_version:  candidate_version,
+            virtual:            virtual,
           }
         end
 
-        def check_all_packages_state(package)
-          installed_version = {}
-          candidate_version = {}
+        def package_data
+          @package_data ||= Hash.new do |hash, key|
+            hash[key] = package_data_for(key)
+          end
+        end
 
-          [package].flatten.each do |pkg|
-            ret = check_package_state(pkg)
-            is_virtual_package[pkg] = ret[:is_virtual_package]
-            installed_version[pkg]  = ret[:installed_version]
-            candidate_version[pkg]  = ret[:candidate_version]
+        def get_current_versions
+          package_name_array.map do |package_name|
+            package_data[package_name][:current_version]
           end
+        end
 
-          if package.is_a?(Array)
-            @candidate_version = []
-            final_installed_version = []
-            [package].flatten.each do |pkg|
-              candidate_version << candidate_version[pkg]
-              final_installed_version << installed_version[pkg]
-            end
-            current_resource.version(final_installed_version)
-          else
-            @candidate_version = candidate_version[package]
-            current_resource.version(installed_version[package])
+        def get_candidate_versions
+          package_name_array.map do |package_name|
+            package_data[package_name][:candidate_version]
           end
         end
 
+        def candidate_version
+          @candidate_version ||= get_candidate_versions
+        end
+
         def install_package(name, version)
           package_name = name.zip(version).map do |n, v|
-            is_virtual_package[n] ? n : "#{n}=#{v}"
+            package_data[n][:virtual] ? n : "#{n}=#{v}"
           end.join(" ")
           run_noninteractive("apt-get -q -y", default_release_options, new_resource.options, "install", package_name)
         end
diff --git a/spec/unit/provider/package/apt_spec.rb b/spec/unit/provider/package/apt_spec.rb
index 3fd48e46e8..98d2b8d524 100644
--- a/spec/unit/provider/package/apt_spec.rb
+++ b/spec/unit/provider/package/apt_spec.rb
@@ -61,7 +61,7 @@ irssi:
           expect(current_resource).to be_a(Chef::Resource::Package)
           expect(current_resource.name).to eq("irssi")
           expect(current_resource.package_name).to eq("irssi")
-          expect(current_resource.version).to be_nil
+          expect(current_resource.version).to eql([nil])
         end
 
         it "should set the installed version if package has one" do
@@ -79,8 +79,8 @@ sudo:
           INSTALLED
           expect(@provider).to receive(:shell_out!).and_return(@shell_out)
           @provider.load_current_resource
-          expect(@provider.current_resource.version).to eq("1.7.2p1-1ubuntu5.3")
-          expect(@provider.candidate_version).to eql("1.7.2p1-1ubuntu5.3")
+          expect(@provider.current_resource.version).to eq(["1.7.2p1-1ubuntu5.3"])
+          expect(@provider.candidate_version).to eql(["1.7.2p1-1ubuntu5.3"])
         end
 
         # libmysqlclient-dev is a real package in newer versions of debian + ubuntu
@@ -210,9 +210,6 @@ mpg123 1.12.1-0ubuntu1
             :env => { "DEBIAN_FRONTEND" => "noninteractive" } ,
             :timeout => @timeout
           ).and_return(@shell_out)
-          @provider.load_current_resource
-          @provider.define_resource_requirements
-          expect(@provider).to receive(:shell_out!).with("apt-cache policy irssi", { :env => { "DEBIAN_FRONTEND" => "noninteractive" }, :timeout => 900 }).and_return(@shell_out)
           expect { @provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package)
         end
       end
@@ -221,6 +218,18 @@ mpg123 1.12.1-0ubuntu1
         before do
           @current_resource = resource_klass.new("irssi", @run_context)
           @provider.current_resource = @current_resource
+          allow(@provider).to receive(:package_data).and_return({
+            "irssi" => {
+              virtual: false,
+              candidate_version: "0.8.12-7",
+              installed_version: nil,
+            },
+            "libmysqlclient-dev" => {
+              virtual: true,
+              candidate_version: nil,
+              installed_version: nil,
+            },
+          })
         end
 
         describe "install_package" do
@@ -364,7 +373,7 @@ mpg123 1.12.1-0ubuntu1
 
         describe "when installing a virtual package" do
           it "should install the package without specifying a version" do
-            @provider.is_virtual_package["libmysqlclient-dev"] = true
+            @provider.package_data["libmysqlclient-dev"][:virtual] = true
             expect(@provider).to receive(:shell_out!).with(
               "apt-get -q -y install libmysqlclient-dev",
               :env => { "DEBIAN_FRONTEND" => "noninteractive" },
@@ -377,8 +386,8 @@ mpg123 1.12.1-0ubuntu1
         describe "when installing multiple packages" do
           it "can install a virtual package followed by a non-virtual package" do
             # https://github.com/chef/chef/issues/2914
-            @provider.is_virtual_package["libmysqlclient-dev"] = true
-            @provider.is_virtual_package["irssi"] = false
+            @provider.package_data["libmysqlclient-dev"][:virtual] = true
+            @provider.package_data["irssi"][:virtual] = false
             expect(@provider).to receive(:shell_out!).with(
               "apt-get -q -y install libmysqlclient-dev irssi=0.8.12-7",
               :env => { "DEBIAN_FRONTEND" => "noninteractive" },