Merge pull request #4277 from chef/mwrock/package

non msi packages must explicitly provide a source attribute on install

9 files changed, 72 insertions, 33 deletions

diff --git a/DOC_CHANGES.md b/DOC_CHANGES.md
index 46ba8cd50f..79d60b856b 100644
--- a/DOC_CHANGES.md
+++ b/DOC_CHANGES.md
@@ -88,11 +88,19 @@ to better align with other ssh related options.
 
 `windows_package` now supports more than just `MSI`. Most common windows installer types are supported including Inno Setup, Nullsoft, Wise and InstallShield. The new allowed `installer_type` values are: `inno`, `nsis`, `wise`, `installshield`, `custom`, and `msi`.
 
+**Non `:msi` package installation**
+When installing non `:msi` packages, the `package_name` should match the display name used for the installed package in the "Add/Remove Programs" settings application. This value can also be found in the following registry locations:
+* HKEY_LOCAL_MACHINE\Software\Microsoft\Windows\CurrentVersion\Uninstall
+* HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Uninstall
+* HKEY_LOCAL_MACHINE\Software\Wow6464Node\Microsoft\Windows\CurrentVersion\Uninstall
+
+ Further, non `:msi` packages must explicitly include a package `source` attribute when using the `:install` action. Unlike `:msi` packages, they will not default to the package name if missing. Without the name matching the software's display name, non `:msi` packages will always reconverge on `:install`.
+
 Also, while being able to download remote installers from a `HTTP` resource is not new, it looks as though the top of the docs page is incorrect stating that only local installers can be used as a source.
 
 Example Nullsoft (`nsis`) package resource:
 ```
-windows_package 'Mercurial 3.6.1 (64-bit)' do
+package 'Mercurial 3.6.1 (64-bit)' do
   source 'http://mercurial.selenic.com/release/windows/Mercurial-3.6.1-x64.exe'
   checksum 'febd29578cb6736163d232708b834a2ddd119aa40abc536b2c313fc5e1b5831d'
 end
@@ -100,7 +108,7 @@ end
 
 Example Custom `windows_package` resource:
 ```
-windows_package 'Microsoft Visual C++ 2005 Redistributable' do
+package 'Microsoft Visual C++ 2005 Redistributable' do
   source 'https://download.microsoft.com/download/6/B/B/6BB661D6-A8AE-4819-B79F-236472F6070C/vcredist_x86.exe'
   installer_type :custom
   options '/Q'
@@ -108,15 +116,18 @@ end
 ```
 Using a `:custom` package is one way to install a non `.msi` file that embeds an `msi` based installer.
 
+**`windows_package` removal**
 Packages can now be removed without the need to include the package `source`. The relevent uninstall metadata will now be discovered from the registry.
 ```
-windows_package 'Mercurial 3.6.1 (64-bit)' do
+package 'Mercurial 3.6.1 (64-bit)' do
   action :remove
 end
 ```
-It is important that the package name used when not including the `source` is EXACTLY the same as the display name found in "Add/Remove programs" or the `DisplayName` property in the appropriate registry key:
+For non `:msi` packages, it is important that the package name used is EXACTLY the same as the display name found in "Add/Remove programs" or the `DisplayName` property in the appropriate registry key:
 * HKEY_LOCAL_MACHINE\Software\Microsoft\Windows\CurrentVersion\Uninstall
 * HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Uninstall
 * HKEY_LOCAL_MACHINE\Software\Wow6464Node\Microsoft\Windows\CurrentVersion\Uninstall
 
+When removing `:msi` packages, this same package naming rule applies if the `source` is omitted.
+
 Note that if there are multiple versions of a package installed with the same display name, all packages will be removed unless a version is provided in the `version` attribute or can be discovered in the `source` installer file.
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 4aa4ae42a3..6aa8c7ab19 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -59,7 +59,7 @@ This is the first release where we are rolling out a MSI package for Windows tha
 
 ## `windows_package` now supports non-`MSI` based Windows installers
 
-Today you can install `MSI`s using the `windows_package` resource. However, you have had to use the windows cookbook in order to install non `MSI` based installer packages such as Nullsoft, Inno Setup, Installshield and other `EXE` based installers. We have moved and slightly improved the windows cookbook resource into the core chef client. This means you can now run most windows installer types without taking on external cookbook dependencies.
+Today you can install `MSI`s using the `windows_package` resource. However, you have had to use the windows cookbook in order to install non `MSI` based installer packages such as Nullsoft, Inno Setup, Installshield and other `EXE` based installers. We have moved and slightly improved the windows cookbook resource into the core chef client. This means you can now run most windows installer types without taking on external cookbook dependencies. Note that the `source` attribute of non `:msi` windows packages is required since the `package_name` is expected to match the same name displayed in "Add/Remove Programs."
 
 ## Better handling of log_location with chef client service (Windows)
 
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
diff --git a/spec/functional/resource/windows_package_spec.rb b/spec/functional/resource/windows_package_spec.rb
index 65378653b0..f5fdc9ef3d 100644
--- a/spec/functional/resource/windows_package_spec.rb
+++ b/spec/functional/resource/windows_package_spec.rb
@@ -29,7 +29,7 @@ describe Chef::Resource::WindowsPackage, :windows_only, :volatile do
 
   subject do
     new_resource = Chef::Resource::WindowsPackage.new(pkg_name, run_context)
-    new_resource.source pkg_path
+    new_resource.source pkg_path if pkg_path
     new_resource.version pkg_version
     new_resource.installer_type pkg_type
     new_resource.options pkg_options
diff --git a/spec/unit/provider/package/windows_spec.rb b/spec/unit/provider/package/windows_spec.rb
index c26c446b5b..6952a91d92 100644
--- a/spec/unit/provider/package/windows_spec.rb
+++ b/spec/unit/provider/package/windows_spec.rb
@@ -33,7 +33,7 @@ describe Chef::Provider::Package::Windows, :windows_only do
   let(:resource_name) { 'calculator' }
   let(:new_resource) do
     new_resource = Chef::Resource::WindowsPackage.new(resource_name)
-    new_resource.source(resource_source)
+    new_resource.source(resource_source) if resource_source
     new_resource
   end
   let(:provider) { Chef::Provider::Package::Windows.new(new_resource, run_context) }
@@ -98,7 +98,7 @@ describe Chef::Provider::Package::Windows, :windows_only do
     shared_examples "a local file" do
 
       it "checks that the source path is valid" do
-        expect(Chef::Util::PathHelper).to receive(:validate_path)
+        expect(Chef::Util::PathHelper).to receive(:validate_path).and_call_original
         provider.package_provider
       end
 
@@ -280,15 +280,25 @@ describe Chef::Provider::Package::Windows, :windows_only do
   end
 
   describe "action_install" do
-    let(:new_resource) { Chef::Resource::WindowsPackage.new("blah.exe") }
+    let(:resource_name) { "blah" }
+    let(:resource_source) { "blah.exe" }
+
     before do
       new_resource.installer_type(:inno)
       allow_any_instance_of(Chef::Provider::Package::Windows::Exe).to receive(:package_version).and_return(new_resource.version)
     end
 
+    context "no source given" do
+      let(:resource_source) { nil }
+
+      it "raises a NoWindowsPackageSource error" do
+        expect { provider.run_action(:install) }.to raise_error(Chef::Exceptions::NoWindowsPackageSource)
+      end
+    end
+
     context "no version given, discovered or installed" do
       it "installs latest" do
-        expect(provider).to receive(:install_package).with("blah.exe", "latest")
+        expect(provider).to receive(:install_package).with("blah", "latest")
         provider.run_action(:install)
       end
     end
@@ -306,7 +316,7 @@ describe Chef::Provider::Package::Windows, :windows_only do
       before { new_resource.version('5.5.5') }
 
       it "installs given version" do
-        expect(provider).to receive(:install_package).with("blah.exe", "5.5.5")
+        expect(provider).to receive(:install_package).with("blah", "5.5.5")
         provider.run_action(:install)
       end
     end
@@ -331,7 +341,7 @@ describe Chef::Provider::Package::Windows, :windows_only do
         end
         
         it "installs given version" do
-          expect(provider).to receive(:install_package).with("blah.exe", "5.5.5")
+          expect(provider).to receive(:install_package).with("blah", "5.5.5")
           provider.run_action(:install)
         end
       end
@@ -357,7 +367,7 @@ describe Chef::Provider::Package::Windows, :windows_only do
         end
         
         it "installs given version" do
-          expect(provider).to receive(:install_package).with("blah.exe", "5.5.5")
+          expect(provider).to receive(:install_package).with("blah", "5.5.5")
           provider.run_action(:install)
         end
       end