Merge pull request #4277 from chef/mwrock/package

non msi packages must explicitly provide a source attribute on install

5 files changed, 38 insertions, 20 deletions

diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index 0f4e74ad11..8736ff7ffc 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -170,6 +170,7 @@ class Chef
 
     class CannotDetermineHomebrewOwner < Package; end
     class CannotDetermineWindowsInstallerType < Package; end
+    class NoWindowsPackageSource < Package; end
 
     # Can not create staging file during file deployment
     class FileContentStagingError < RuntimeError
diff --git a/lib/chef/provider/package/windows.rb b/lib/chef/provider/package/windows.rb
index b4391774ca..fc90079846 100644
--- a/lib/chef/provider/package/windows.rb
+++ b/lib/chef/provider/package/windows.rb
@@ -34,15 +34,22 @@ class Chef
 
         require 'chef/provider/package/windows/registry_uninstall_entry.rb'
 
+        def define_resource_requirements
+          requirements.assert(:install) do |a|
+            a.assertion { new_resource.source unless package_provider == :msi }
+            a.failure_message Chef::Exceptions::NoWindowsPackageSource, "Source for package #{new_resource.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."
+          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)
+          @current_resource = Chef::Resource::WindowsPackage.new(new_resource.name)
           if downloadable_file_missing?
             Chef::Log.debug("We do not know the version of #{new_resource.source} because the file is not downloaded")
             current_resource.version(:unknown.to_s)
           else
             current_resource.version(package_provider.installed_version)
-            new_resource.version(package_provider.package_version)
+            new_resource.version(package_provider.package_version) if package_provider.package_version
           end
 
           current_resource
@@ -52,11 +59,11 @@ class Chef
           @package_provider ||= begin
             case installer_type
             when :msi
-              Chef::Log.debug("#{@new_resource} is MSI")
+              Chef::Log.debug("#{new_resource} is MSI")
               require 'chef/provider/package/windows/msi'
               Chef::Provider::Package::Windows::MSI.new(resource_for_provider, uninstall_registry_entries)
             else
-              Chef::Log.debug("#{@new_resource} is EXE with type '#{installer_type}'")
+              Chef::Log.debug("#{new_resource} is EXE with type '#{installer_type}'")
               require 'chef/provider/package/windows/exe'
               Chef::Provider::Package::Windows::Exe.new(resource_for_provider, installer_type, uninstall_registry_entries)
             end
@@ -69,8 +76,8 @@ class Chef
           # binary to determine the installer type for the user. Since the file
           # must be on disk to do so, we have to make this choice in the provider.
           @installer_type ||= begin
-            if @new_resource.installer_type
-              @new_resource.installer_type
+            if new_resource.installer_type
+              new_resource.installer_type
             elsif source_location.nil?
                 inferred_registry_type
             else
@@ -108,7 +115,7 @@ class Chef
                 if basename == 'setup.exe'
                   :installshield
                 else
-                  fail Chef::Exceptions::CannotDetermineWindowsInstallerType, "Installer type for Windows Package '#{@new_resource.name}' not specified and cannot be determined from file extension '#{file_extension}'"
+                  fail Chef::Exceptions::CannotDetermineWindowsInstallerType, "Installer type for Windows Package '#{new_resource.name}' not specified and cannot be determined from file extension '#{file_extension}'"
                 end
               end
             end
@@ -144,7 +151,7 @@ class Chef
 
         # @return [String] candidate_version
         def candidate_version
-          @candidate_version ||= (@new_resource.version || 'latest')
+          @candidate_version ||= (new_resource.version || 'latest')
         end
 
         # @return [Array] current_version(s) as an array
@@ -160,7 +167,7 @@ class Chef
         #
         # @return [Boolean] true if new_version is equal to or included in current_version
         def target_version_already_installed?(current_version, new_version)
-          Chef::Log.debug("Checking if #{@new_resource} version '#{new_version}' is already installed. #{current_version} is currently installed")
+          Chef::Log.debug("Checking if #{new_resource} version '#{new_version}' is already installed. #{current_version} is currently installed")
           if current_version.is_a?(Array)
             current_version.include?(new_version)
           else
@@ -175,7 +182,7 @@ class Chef
         private
 
         def uninstall_registry_entries
-          @uninstall_registry_entries ||= Chef::Provider::Package::Windows::RegistryUninstallEntry.find_entries(new_resource.name)
+          @uninstall_registry_entries ||= Chef::Provider::Package::Windows::RegistryUninstallEntry.find_entries(new_resource.package_name)
         end
 
         def inferred_registry_type
@@ -188,7 +195,7 @@ class Chef
         end
 
         def downloadable_file_missing?
-          uri_scheme?(new_resource.source) && !::File.exists?(source_location)
+          !new_resource.source.nil? && uri_scheme?(new_resource.source) && !::File.exists?(source_location)
         end
 
         def resource_for_provider
@@ -203,7 +210,7 @@ class Chef
 
         def download_source_file
           source_resource.run_action(:create)
-          Chef::Log.debug("#{@new_resource} fetched source file to #{source_resource.path}")
+          Chef::Log.debug("#{new_resource} fetched source file to #{source_resource.path}")
         end
 
         def source_resource
@@ -228,7 +235,9 @@ class Chef
         end
 
         def source_location
-          if uri_scheme?(new_resource.source)
+          if new_resource.source.nil?
+            nil
+          elsif uri_scheme?(new_resource.source)
             source_resource.path
           else
             new_source = Chef::Util::PathHelper.cleanpath(new_resource.source)
diff --git a/lib/chef/provider/package/windows/exe.rb b/lib/chef/provider/package/windows/exe.rb
index 4495868010..3758b8b1e2 100644
--- a/lib/chef/provider/package/windows/exe.rb
+++ b/lib/chef/provider/package/windows/exe.rb
@@ -99,7 +99,7 @@ class Chef
 
           def install_file_version
             @install_file_version ||= begin
-              if ::File.exist?(@new_resource.source)
+              if !new_resource.source.nil? && ::File.exist?(new_resource.source)
                 version_info = Chef::ReservedNames::Win32::File.version_info(new_resource.source)
                 file_version = version_info.FileVersion || version_info.ProductVersion
                 file_version == '' ? nil : file_version
diff --git a/lib/chef/provider/package/windows/msi.rb b/lib/chef/provider/package/windows/msi.rb
index 1cc636b92e..15b00f46f0 100644
--- a/lib/chef/provider/package/windows/msi.rb
+++ b/lib/chef/provider/package/windows/msi.rb
@@ -44,7 +44,7 @@ class Chef
 
           # Returns a version if the package is installed or nil if it is not.
           def installed_version
-            if ::File.exist?(new_resource.source)
+            if !new_resource.source.nil? && ::File.exist?(new_resource.source)
               Chef::Log.debug("#{new_resource} getting product code for package at #{new_resource.source}")
               product_code = get_product_property(new_resource.source, "ProductCode")
               Chef::Log.debug("#{new_resource} checking package status and version for #{product_code}")
@@ -58,7 +58,7 @@ class Chef
 
           def package_version
             return new_resource.version if new_resource.version
-            if ::File.exist?(new_resource.source)
+            if !new_resource.source.nil? && ::File.exist?(new_resource.source)
               Chef::Log.debug("#{new_resource} getting product version for package at #{new_resource.source}")
               get_product_property(new_resource.source, "ProductVersion")
             end
@@ -72,7 +72,7 @@ class Chef
 
           def remove_package
             # We could use MsiConfigureProduct here, but we'll start off with msiexec
-            if ::File.exist?(new_resource.source)
+            if !new_resource.source.nil? && ::File.exist?(new_resource.source)
               Chef::Log.debug("#{new_resource} removing MSI package '#{new_resource.source}'")
               shell_out!("msiexec /qn /x \"#{new_resource.source}\" #{expand_options(new_resource.options)}", {:timeout => new_resource.timeout, :returns => new_resource.returns})
             else
diff --git a/lib/chef/resource/windows_package.rb b/lib/chef/resource/windows_package.rb
index 4965f3f117..7ccf95f653 100644
--- a/lib/chef/resource/windows_package.rb
+++ b/lib/chef/resource/windows_package.rb
@@ -32,16 +32,24 @@ class Chef
 
       allowed_actions :install, :remove
 
+      def initialize(name, run_context=nil)
+        super
+        @source ||= source(@package_name) if @package_name.downcase.end_with?('.msi')
+      end
+
       # Unique to this resource
       property :installer_type, Symbol
       property :timeout, [ String, Integer ], default: 600
       # In the past we accepted return code 127 for an unknown reason and 42 because of a bug
       property :returns, [ String, Integer, Array ], default: [ 0 ], desired_state: false
-      property :source, String, name_property: true,
-                coerce: proc { |s| uri_scheme?(s) ? s : Chef::Util::PathHelper.canonical_path(s, false) }
+      property :source, String,
+                coerce: (proc do |s|
+                  unless s.nil?
+                    uri_scheme?(s) ? s : Chef::Util::PathHelper.canonical_path(s, false)
+                  end
+                end)
       property :checksum, String, desired_state: false
       property :remote_file_attributes, Hash, desired_state: false
-
     end
   end
 end