diff options
author | Tim Smith <tsmith@chef.io> | 2020-05-04 14:23:35 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-05-04 14:23:35 -0700 |
commit | 01b591da5149f7dad6b3e285e5ca450776f225d8 (patch) | |
tree | bdfa01677a4545697945727e3fe94ca5b660139c | |
parent | cf8e24e13e3bf3e8a06db8b2099cb06d3e67d666 (diff) | |
parent | 7235856d859bbb32482b3f3b12f758b63a0eb8eb (diff) | |
download | chef-01b591da5149f7dad6b3e285e5ca450776f225d8.tar.gz |
Merge pull request #9790 from chef/lcg/fix-windows-package
Fix windows package super bug and cleanup
-rw-r--r-- | lib/chef/provider/package/windows.rb | 77 |
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 |