fix: Do not hard code package name in puppet_agent_end_run.rb

[fetzerms]

When using this module to install puppet-agent compatible packages with a different name (such as openvox-agent) calling puppet_agent_end_run with latest will fail, as the package name is hard coded.

This patch replaces the hard coded puppet-agent with a dynamic lookup for the package name. It also considers that (during migration) the latest_version lookup will not work.

How to reproduce the error:

    class { '::puppet_agent':
      package_version => 'latest',
      package_name    => 'openvox-agent',
      manage_repo     => false,
      require         => [
        Apt::Source['openvox']
      ],
    }

Results in:

Error: undefined method `parameters' for nil:NilClass
Info: Unknown failure using insync_values? on type: Puppet_agent_end_run[latest] / property: end_run to compare values [true] and false
Error: /Stage[main]/Puppet_agent::Install/Puppet_agent_end_run[latest]/end_run: change from false to true failed: undefined method `parameters' for nil:NilClass

[CLAassistant]

All committers have signed the CLA.

[fetzerms]

Not sure how to handle the tests, as they do not seem to ensure that the puppet_agent class is in the catalog. Most likely adding to catalog in puppet_agent_end_run_spec? And also adding other "bogus" packages to ensure it works properly?

[joshcooper]

If latest_version is nil, then desired_version will be too, and the versioncmp function will raise:

❯ bundle exec ruby -rpuppet -e 'Puppet::Util::Package.versioncmp(nil, "1.2.3")' 
/home/josh/work/puppet-private/lib/puppet/util/package.rb:12:in `versioncmp': undefined method `scan' for nil:NilClass (NoMethodError)

Maybe use empty string instead?

desired_version = if latest_version.nil?
                    ""
                  else
                    latest_version.match(%r{^(?:[0-9]:)?(\d+\.\d+(\.\d+)?(?:\.\d+))?}).captures.first
                  end

[fetzerms]

If latest_version is nil then desired_version will still be 'latest' - But I think it makes sense to be more explicit here.

[joshcooper]

If package_name can be nil (not sure that's possible?), then it'd be good to set a default:

Suggested change

	package_name = @resource.catalog.resource('class', 'puppet_agent')[:package_name]
	package_name = @resource.catalog.resource('class', 'puppet_agent')[:package_name] || 'puppet-agent'

[fetzerms]

I don't think it can (realistically) be nil - but having a sane default still seems like a good idea.

[joshcooper]

The tests are failing because they "manually" create a catalog and add the agent_latest_package to it. You could add the class too, it's a little gross. Note the differences with dashes and underscores.

       Puppet::Type.type(:package).new(name: 'puppet-agent', ensure: 'latest', provider: :yum)
     end
 
+    let(:puppet_agent_class) do
+      Puppet::Type.type(:component).new(name: 'puppet_agent', package_name: 'puppet-agent')
+    end
+
     before(:each) do
+      catalog.add_resource(puppet_agent_class)
       catalog.add_resource(agent_latest_package)
       resource.catalog = catalog
     end

[fetzerms]

The tests are failing because they "manually" create a catalog and add the agent_latest_package to it. You could add the class too, it's a little gross. Note the differences with dashes and underscores.

       Puppet::Type.type(:package).new(name: 'puppet-agent', ensure: 'latest', provider: :yum)
     end
 
+    let(:puppet_agent_class) do
+      Puppet::Type.type(:component).new(name: 'puppet_agent', package_name: 'puppet-agent')
+    end
+
     before(:each) do
+      catalog.add_resource(puppet_agent_class)
       catalog.add_resource(agent_latest_package)
       resource.catalog = catalog
     end

Thanks a lot! I'll enhance/fix the tests and will include your previous review comments early next week.