fix ensure_packages duplicate checking

[netzvieh]

Instead of just looking for an already existing package resource with
the same name, we should check for the parameters, too. Otherwise having
one ensure_packages call with ensure => present and one with ensure =>
absent leads to the second function call being ignored. It should throw
a duplicate declaration error instead.

Since we are already calling ensure_resource internally and
ensure_resource itself does check for duplicates, we don't need that
extra check in ensure_packages.

[netzvieh]

ensure_resource and ensure_resources throw the error as expected:

[root@a2e375cd0c1c ~]# cat ensure_resource.pp
ensure_resource('package', 'vim-enhanced', { 'ensure' => 'present' })
ensure_resource('package', 'vim-enhanced', { 'ensure' => 'present' })
ensure_resource('package', 'vim-enhanced', { 'ensure' => 'absent' })
[root@a2e375cd0c1c ~]# puppet apply ensure_resource.pp 
Error: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[vim-enhanced] is already declared at (file: /root/ensure_resource.pp, line: 1); cannot redeclare (file: /root/ensure_resource.pp, line: 3) (file: /root/ensure_resource.pp, line: 3, column: 1) on node a2e375cd0c1c

[root@a2e375cd0c1c ~]# cat ensure_resources.pp
ensure_resources('package', { 'vim-enhanced' => { 'ensure' => 'present' } }, { 'ensure' => 'present' })
ensure_resources('package', { 'vim-enhanced' => { 'ensure' => 'present' } }, { 'ensure' => 'present' })
ensure_resources('package', { 'vim-enhanced' => { 'ensure' => 'absent' } }, { 'ensure' => 'present' })
[root@a2e375cd0c1c ~]# puppet apply ensure_resources.pp
Error: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[vim-enhanced] is already declared at (file: /root/ensure_resources.pp, line: 1); cannot redeclare (file: /root/ensure_resources.pp, line: 3) (file: /root/ensure_resources.pp, line: 3, column: 1) on node a2e375cd0c1c

ensure_packages did not:

[root@a2e375cd0c1c ~]# cat ensure_packages.pp
ensure_packages(['vim-enhanced'], { 'ensure' => 'present' })
ensure_packages(['vim-enhanced'], { 'ensure' => 'present' })
ensure_packages(['vim-enhanced'], { 'ensure' => 'absent' })
[root@a2e375cd0c1c ~]# puppet apply ensure_packages.pp 
Notice: Compiled catalog for a2e375cd0c1c in environment production in 0.05 seconds
Notice: Applied catalog in 0.06 seconds

but does with this patch:

[root@a2e375cd0c1c ~]# cat ensure_packages.pp
ensure_packages(['vim-enhanced'], { 'ensure' => 'present' })
ensure_packages(['vim-enhanced'], { 'ensure' => 'present' })
ensure_packages(['vim-enhanced'], { 'ensure' => 'absent' })
[root@a2e375cd0c1c ~]# puppet apply ensure_packages.pp
Error: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[vim-enhanced] is already declared at (file: /root/ensure_packages.pp, line: 1); cannot redeclare (file: /root/ensure_packages.pp, line: 3) (file: /root/ensure_packages.pp, line: 3, column: 1) on node a2e375cd0c1c

[HelenCampbell]

This looks great, thank you for your contribution!

[alexjfisher]

Some people are relying on the old behaviour though. It was introduced in #735

[amateo]

I'm having problems to update to 5.2.0 from 5.1.0.

My code is:

class profile::base (
  $packages = {},
  ...
) {
  $packages.each |$name, $opts| {
  package {$name:
    * => $opts,
  }
  ...
}

class profile::vcsrepo {
  ensure_packages('git')
  ...
}

now I have problems with a node that contains git in its profile::base::packages parameter.
Until now (puppetlabs/stdlib version 5.1.0) it was working fine, but now I get:

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[git] is already declared at (file: /etc/puppetlabs/code/environments/production/manifests/profile/manifests/base.pp, line: 40); cannot redeclare (file: /etc/puppetlabs/code/environments/production/manifests/profile/manifests/vcsrepo.pp, line: 64) (file: /etc/puppetlabs/code/environments/production/manifests/profile/manifests/vcsrepo.pp, line: 64, column: 7) on node elephas42.um.es

I think that if there is no conflict between the package and ensure_packages declarations, ensure_packages shouldn't generate any error.

[alexjfisher]

4.16.0 Changelog has under bugfixes

Ensure_packages function - Now only tries to apply the resource if not defined.

5.2.0 Changelog under Fixed

ensure-packages() duplicate checking now works as it should.

The 'correct' behaviour seems to be based on individual opinion. I'm not sure changing the behaviour in a minor release was correct though. :)

[amateo]

You are right about the 'correct' behaviour.
I lost the changelog at revision 4.16.0.

But my question now (sure offtopic), then what is the purpose of ensure_packages. I mean, if you already have declared a package resource, then it fails with a duplicate declaration, so what is the difference between package and ensure_packages?

[netzvieh]

Hi @amateo , I had a look at the surrounding code and I think the problem lies in defined_with_params.rb.

If your resource is created without any params, e.g. because $opts is empty, defined_with_params.rb thinks it's a different resource:

package { 'git':
  ensure => present,
}

ensure_packages('git')

The example above works just fine. The example below doesn't.

package{ 'git': }

ensure_package('git')

Because in defined_with_params.rb the code is as follows (debug added by me):

     matches = params.map do |key, value|
       Puppet.debug("Key: #{key}, Value: #{value}, ResourceValue: #{resource[key]}")
       # eql? avoids bugs caused by monkeypatching in puppet
       resource_is_undef = resource[key].eql?(:undef) || resource[key].nil?
       value_is_undef = value.eql?(:undef) || value.nil?
       (resource_is_undef && value_is_undef) || (resource[key] == value)
     end

So what happens now is:

Debug: Key: ensure, Value: present, ResourceValue: 

And with that resource[key] == value is false and the function returns, that the resource doesn't exist yet. Imho the correct solution would be puppet setting the defaults as resource params when defining a resource. E.g. that even if it's just package { 'git': } in the manifest, the representation should automatically be package { 'git': ensure => present }.

We probably need an issue for this though, since yes, this is somewhat of a regression and I didn't test the case of packages without parameters..

[netzvieh]

An alternative would be, for ensure_packages() to call ensure_resource() without setting the params to { ensure => present } per default. defined_with_params() always returns true if the resource exists and the parameter hash is empty.

But in that case ensure_packages('git') would not really ensure, that the package git is installed if it was defined with ensure => absent earlier.

[netzvieh]

@HelenCampbell what do you think?