Make ensure_packages work with ensure => present

This unbreaks the breaking change made in #1196

Also refactored to create a separate dispatch method for the case when
`packages` is a `Hash`, and having that call the main `ensure_packages`
method. This simplifies the code by only ever calling `ensure_resource`
instead of calling `ensure_resources` for hashes.

Defaulting `default_attributes` to an empty hash instead of `nil` in the
method signatures further simplifies the code.

Fixes #1252

Author: alexjfisher

lib/puppet/functions/ensure_packages.rb

@@ -6,34 +6,56 @@
 # third argument to the ensure_resource() function.
 Puppet::Functions.create_function(:ensure_packages, Puppet::Functions::InternalFunction) do
   # @param packages
-  #   The packages to ensure are installed. If it's a Hash it will be passed to `ensure_resource`
+  #   The packages to ensure are installed.
   # @param default_attributes
   #   Default attributes to be passed to the `ensure_resource()` function
   # @return [Undef] Returns nothing.
   dispatch :ensure_packages do
     scope_param
-    param 'Variant[String[1], Array[String[1]], Hash[String[1], Any]]', :packages
+    param 'Variant[String[1], Array[String[1]]]', :packages
     optional_param 'Hash', :default_attributes
     return_type 'Undef'
   end
 
-  def ensure_packages(scope, packages, default_attributes = nil)
-    if default_attributes
+  # @param packages
+  #   The packages to ensure are installed. The keys are packages and values are the attributes specific to that package.
+  # @param default_attributes
+  #   Default attributes. Package specific attributes from the `packages` parameter will take precedence.
+  # @return [Undef] Returns nothing.
+  dispatch :ensure_packages_hash do
+    scope_param
+    param 'Hash[String[1], Any]', :packages
+    optional_param 'Hash', :default_attributes
+    return_type 'Undef'
+  end
+
+  def ensure_packages(scope, packages, default_attributes = {})
+    Array(packages).each do |package_name|
       defaults = { 'ensure' => 'installed' }.merge(default_attributes)
-      if defaults['ensure'] == 'present'
-        defaults['ensure'] = 'installed'
-      end
-    else
-      defaults = { 'ensure' => 'installed' }
+
+      # `present` and `installed` are aliases for the `ensure` attribute. If `ensure` is set to either of these values replace
+      # with `installed` by default but `present` if this package is already in the catalog with `ensure => present`
+      defaults['ensure'] = default_ensure(package_name) if ['present', 'installed'].include?(defaults['ensure'])
+
+      scope.call_function('ensure_resource', ['package', package_name, defaults])
     end
+    nil
+  end
 
-    if packages.is_a?(Hash)
-      scope.call_function('ensure_resources', ['package', packages.dup, defaults])
-    else
-      Array(packages).each do |package_name|
-        scope.call_function('ensure_resource', ['package', package_name, defaults])
-      end
+  def ensure_packages_hash(scope, packages, default_attributes = {})
+    packages.each do |package, attributes|
+      ensure_packages(scope, package, default_attributes.merge(attributes))
     end
     nil
   end
+
+  private
+
+  def default_ensure(package_name)
+    if call_function('defined_with_params', "Package[#{package_name}]", { 'ensure' => 'present' })
+      'present'
+    else
+      'installed'
+    end
+  end
 end

spec/functions/ensure_packages_spec.rb

@@ -32,6 +32,14 @@
       subject.execute({ 'foo' => { 'provider' => 'rpm' }, 'bar' => { 'provider' => 'gem' } }, 'ensure' => 'present')
       subject.execute('パッケージ' => { 'ensure' => 'absent' })
       subject.execute('ρǻ¢κầģẻ' => { 'ensure' => 'absent' })
+      subject.execute(
+        {
+          'package_one'   => {},
+          'package_two'   => {},
+          'package_three' => { 'provider' => 'puppetserver_gem' },
+        },
+        { 'provider' => 'puppet_gem' },
+      )
     end
 
     # this lambda is required due to strangeness within rspec-puppet's expectation handling
@@ -42,6 +50,14 @@
       it { expect(-> { catalogue }).to contain_package('パッケージ').with('ensure' => 'absent') }
       it { expect(-> { catalogue }).to contain_package('ρǻ¢κầģẻ').with('ensure' => 'absent') }
     end
+
+    describe 'default attributes' do
+      it 'package specific attributes take precedence' do
+        expect(-> { catalogue }).to contain_package('package_one').with('provider' => 'puppet_gem')
+        expect(-> { catalogue }).to contain_package('package_two').with('provider' => 'puppet_gem')
+        expect(-> { catalogue }).to contain_package('package_three').with('provider' => 'puppetserver_gem')
+      end
+    end
   end
 
   context 'when given a catalog with "package { puppet: ensure => installed }"' do
@@ -54,4 +70,26 @@
       it { expect(-> { catalogue }).to contain_package('puppet').with_ensure('installed') }
     end
   end
+
+  context 'when given a catalog with "package { puppet: ensure => present }"' do
+    let(:pre_condition) { 'package { puppet: ensure => present }' }
+
+    describe 'after running ensure_package("puppet", { "ensure" => "present" })' do
+      before(:each) { subject.execute('puppet', 'ensure' => 'present') }
+
+      it { expect(-> { catalogue }).to contain_package('puppet').with_ensure('present') }
+    end
+
+    describe 'after running ensure_package("puppet", { "ensure" => "installed" })' do
+      before(:each) { subject.execute('puppet', 'ensure' => 'installed') }
+
+      it { expect(-> { catalogue }).to contain_package('puppet').with_ensure('present') }
+    end
+
+    describe 'after running ensure_package(["puppet"])' do
+      before(:each) { subject.execute(['puppet']) }
+
+      it { expect(-> { catalogue }).to contain_package('puppet').with_ensure('present') }
+    end
+  end
 end