diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2020-05-04 13:29:40 -0700 |
---|---|---|
committer | Lamont Granquist <lamont@scriptkiddie.org> | 2020-05-04 13:29:40 -0700 |
commit | 609a7d70f8b581f3a722ceb5adcebbbed402ba20 (patch) | |
tree | 711f14d434b36478709d506715637c39a8d76173 | |
parent | cf8e24e13e3bf3e8a06db8b2099cb06d3e67d666 (diff) | |
download | chef-609a7d70f8b581f3a722ceb5adcebbbed402ba20.tar.gz |
Fix windows package super bug and cleanup
Remove the overriding of the action block entirely and clean up some
overly complicated code.
In the process it looks like the "unknown" version codepath is blatantly
entirely unused, so it was removed. A bunch of extra unnecessary
indirection was also removed.
Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
-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..964479b15d 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 #{source_resource.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 |