Merge pull request #9790 from chef/lcg/fix-windows-package

Fix windows package super bug and cleanup

1 files changed, 26 insertions, 51 deletions

diff --git a/lib/chef/provider/package/windows.rb b/lib/chef/provider/package/windows.rb
index d5b49a050e..7c77461499 100644
--- a/lib/chef/provider/package/windows.rb
+++ b/lib/chef/provider/package/windows.rb
@@ -36,6 +36,13 @@ class Chef
         require "chef/provider/package/windows/registry_uninstall_entry.rb"
 
         def define_resource_requirements
+          if new_resource.checksum
+            requirements.assert(:install) do |a|
+              a.assertion { new_resource.checksum.downcase == checksum(source_location) }
+              a.failure_message Chef::Exceptions::Package, "Checksum on resource (#{short_cksum(new_resource.checksum)}) does not match checksum on content (#{short_cksum(source_location)})"
+            end
+          end
+
           requirements.assert(:install) do |a|
             a.assertion { new_resource.source || msi? }
             a.failure_message Chef::Exceptions::NoWindowsPackageSource, "Source for package #{new_resource.package_name} must be specified in the resource's source property for package to be installed because the package_name property is used to test for the package installation state for this package type."
@@ -50,27 +57,15 @@ class Chef
           end
         end
 
-        # load_current_resource is run in Chef::Provider#run_action when not in whyrun_mode?
         def load_current_resource
-          @current_resource = Chef::Resource::WindowsPackage.new(new_resource.name)
-          if downloadable_file_missing?
-            logger.trace("We do not know the version of #{new_resource.source} because the file is not downloaded")
-            # FIXME: this label should not be used.  It could be set to nil.  Probably what should happen is that
-            # if the file hasn't been downloaded then load_current_resource must download the file here, and then
-            # the else clause to set current_resource.version can always be run.  Relying on a side-effect here
-            # produces at least less readable code, if not outright buggy...  (and I'm assuming that this isn't
-            # wholly just a bug -- since if we only need the package_name to determine if its installed then we
-            # need this, so I'm assuming we need to download the file to pull out the name in order to check
-            # the registry -- which it still feels like we get wrong in the sense we're forcing always downloading
-            # and then always installing(?) which violates idempotency -- and I'm having to think way too hard
-            # about this and would need to go surfing around the code to determine what actually happens, probably
-            # in every different package_provider...)
-            current_resource.version(:unknown.to_s)
-          else
-            current_resource.version(package_provider.installed_version)
-            new_resource.version(package_provider.package_version) if package_provider.package_version
+          if uri_scheme?(new_resource.source) && action == :install
+            download_source_file
           end
 
+          @current_resource = Chef::Resource::WindowsPackage.new(new_resource.name)
+          current_resource.version(package_provider.installed_version)
+          new_resource.version(package_provider.package_version) if package_provider.package_version
+
           current_resource
         end
 
@@ -139,17 +134,6 @@ class Chef
           end
         end
 
-        action :install do
-          if uri_scheme?(new_resource.source)
-            download_source_file
-            load_current_resource
-          else
-            validate_content!
-          end
-
-          super
-        end
-
         # Chef::Provider::Package action_install + action_remove call install_package + remove_package
         # Pass those calls to the correct sub-provider
         def install_package(name, version)
@@ -245,6 +229,7 @@ class Chef
         end
 
         def resource_for_provider
+          # XXX: this is crazy
           @resource_for_provider = Chef::Resource::WindowsPackage.new(new_resource.name).tap do |r|
             r.source(Chef::Util::PathHelper.validate_path(source_location)) unless source_location.nil?
             r.cookbook_name = new_resource.cookbook_name
@@ -257,23 +242,22 @@ class Chef
         end
 
         def download_source_file
-          source_resource.run_action(:create)
-          logger.trace("#{new_resource} fetched source file to #{source_resource.path}")
-        end
-
-        def source_resource
-          @source_resource ||= Chef::Resource::RemoteFile.new(default_download_cache_path, run_context).tap do |r|
-            r.source(new_resource.source)
-            r.cookbook_name = new_resource.cookbook_name
-            r.checksum(new_resource.checksum)
-            r.backup(false)
+          # It seems correct that this is a build_resource rather than declare_resource/DSL use since updating should not trigger a notification
+          # unless the downloaded file is actually installed.  The case where the remote_file downloads the package but the package is already
+          # installed on the target should not trigger a notification since the running state did not change.
+          build_resource(:remote_file, default_download_cache_path) do
+            source(new_resource.source)
+            cookbook_name = new_resource.cookbook_name
+            checksum(new_resource.checksum)
+            backup(false)
 
             if new_resource.remote_file_attributes
               new_resource.remote_file_attributes.each do |(k, v)|
-                r.send(k.to_sym, v)
+                send(k.to_sym, v)
               end
             end
-          end
+          end.run_action(:create)
+          logger.trace("#{new_resource} fetched source file to #{default_download_cache_path}")
         end
 
         def default_download_cache_path
@@ -287,22 +271,13 @@ class Chef
           if new_resource.source.nil?
             nil
           elsif uri_scheme?(new_resource.source)
-            source_resource.path
+            default_download_cache_path
           else
             new_source = Chef::Util::PathHelper.cleanpath(new_resource.source)
             ::File.exist?(new_source) ? new_source : nil
           end
         end
 
-        def validate_content!
-          if new_resource.checksum
-            source_checksum = checksum(source_location)
-            if new_resource.checksum.downcase != source_checksum
-              raise Chef::Exceptions::ChecksumMismatch.new(short_cksum(new_resource.checksum), short_cksum(source_checksum))
-            end
-          end
-        end
-
         def msi?
           return true if new_resource.installer_type == :msi