Merge pull request #6100 from chef/smurawski/cab_compare

cab_package should comare package name and version, not just name

2 files changed, 20 insertions, 15 deletions

diff --git a/lib/chef/provider/package/cab.rb b/lib/chef/provider/package/cab.rb
index 3a8cc507f9..21ea95b85e 100644
--- a/lib/chef/provider/package/cab.rb
+++ b/lib/chef/provider/package/cab.rb
@@ -81,34 +81,39 @@ class Chef
         end
 
         def installed_version
-          stdout = dism_command("/Get-PackageInfo /PackagePath:\"#{cab_file_source}\"").stdout
-          package_info = parse_dism_get_package_info(stdout)
           # e.g. Package_for_KB2975719~31bf3856ad364e35~amd64~~6.3.1.8
-          package = split_package_identity(package_info["package_information"]["package_identity"])
+          package = new_cab_identity
           # Search for just the package name to catch a different version being installed
           Chef::Log.debug("#{new_resource} searching for installed package #{package['name']}")
-          found_packages = installed_packages.select { |p| p["package_identity"] =~ /^#{package['name']}~/ }
+          existing_package_identities = installed_packages.map do |p|
+            split_package_identity(p["package_identity"])
+          end
+          found_packages = existing_package_identities.select do |existing_package_ident|
+            existing_package_ident["name"] == package["name"]
+          end
           if found_packages.empty?
             nil
           elsif found_packages.length == 1
-            stdout = dism_command("/Get-PackageInfo /PackageName:\"#{found_packages.first['package_identity']}\"").stdout
-            find_version(stdout)
+            found_packages.first["version"]
           else
             # Presuming this won't happen, otherwise we need to handle it
             raise Chef::Exceptions::Package, "Found multiple packages installed matching name #{package['name']}, found: #{found_packages.length} matches"
           end
         end
 
-        def package_version
-          Chef::Log.debug("#{new_resource} getting product version for package at #{cab_file_source}")
+        def cab_identity_from_cab_file
           stdout = dism_command("/Get-PackageInfo /PackagePath:\"#{cab_file_source}\"").stdout
-          find_version(stdout)
+          package_info = parse_dism_get_package_info(stdout)
+          split_package_identity(package_info["package_information"]["package_identity"])
         end
 
-        def find_version(stdout)
-          package_info = parse_dism_get_package_info(stdout)
-          package = split_package_identity(package_info["package_information"]["package_identity"])
-          package["version"]
+        def new_cab_identity
+          Chef::Log.debug("#{new_resource} getting product version for package at #{cab_file_source}")
+          @new_cab_identity ||= cab_identity_from_cab_file
+        end
+
+        def package_version
+          new_cab_identity["version"].chomp
         end
 
         # returns a hash of package state information given the output of dism /get-packages
diff --git a/lib/chef/provider/package/msu.rb b/lib/chef/provider/package/msu.rb
index 15e18feba4..5c398149ba 100644
--- a/lib/chef/provider/package/msu.rb
+++ b/lib/chef/provider/package/msu.rb
@@ -78,7 +78,7 @@ class Chef
         def get_cab_package(cab_file)
           cab_resource = new_resource
           cab_resource.source = cab_file
-          cab_pkg = Chef::Provider::Package::Cab.new(cab_resource, nil)
+          Chef::Provider::Package::Cab.new(cab_resource, nil)
         end
 
         def download_source_file
@@ -141,7 +141,7 @@ class Chef
           else
             # msu package contains only single xml file. So using xml_files.first is sufficient
             doc = ::File.open(xml_files.first.to_s) { |f| REXML::Document.new f }
-            locations = doc.elements.each("unattend/servicing/package/source") { |element| puts element.attributes["location"] }
+            locations = doc.elements.each("unattend/servicing/package/source") { |element| element.attributes["location"] }
             locations.each do |loc|
               cab_files << msu_dir + "/" + loc.attribute("location").value.split("\\")[1]
             end