puppet-collectd icon indicating copy to clipboard operation
puppet-collectd copied to clipboard

Ensure plugin version is same version as main collectd package

Open markasammut opened this issue 4 years ago • 10 comments

Pull Request (PR) description

If you attempt to specify the collectd version in Hiera, and a newer version is available, issue might be caused because the package install of the plugins on new hosts would install the latest version for both the plugin and the main package (as a dependency). Puppet then starts failing attempting to downgrade the main package to your specified version.

With this PR it is ensure that mysql, snmp, and curl, are installed with the same version as the core package. Same thing was done for the disk plugin some time ago: https://github.com/voxpupuli/puppet-collectd/blob/master/manifests/plugin/disk.pp#L26L37

This Pull Request (PR) fixes the following issues

I don't believe the issue has yet been documented. Ideally this is done for all plugins.

markasammut avatar Sep 16 '21 11:09 markasammut

Make sense :+1:. Can do this the same way for all modules, a quick check show that some bits are missing in at least:

romain@zappy puppet-collectd % grep -r --files-without-match ensure_real manifests/plugin | xargs grep --files-with-match package
manifests/plugin/curl.pp
manifests/plugin/amqp.pp
manifests/plugin/varnish.pp
manifests/plugin/ovs_stats.pp
manifests/plugin/dbi.pp
manifests/plugin/netlink.pp
manifests/plugin/ipmi.pp
manifests/plugin/write_riemann.pp
manifests/plugin/turbostat.pp
manifests/plugin/rrdcached.pp
manifests/plugin/smart.pp
manifests/plugin/dns.pp
manifests/plugin/ping.pp
manifests/plugin/genericjmx.pp
manifests/plugin/rrdtool.pp
manifests/plugin/curl_json.pp
manifests/plugin/redis.pp
manifests/plugin/sysevent.pp
manifests/plugin/sensors.pp
manifests/plugin/iscdhcp.pp
manifests/plugin/procevent.pp
manifests/plugin/postgresql.pp
manifests/plugin/snmp.pp
manifests/plugin/cuda.pp
manifests/plugin/connectivity.pp
manifests/plugin/java.pp
manifests/plugin/snmp_agent.pp
manifests/plugin/ceph.pp
manifests/plugin/amqp1.pp
manifests/plugin/perl.pp
manifests/plugin/mysql.pp
manifests/plugin/apache.pp
manifests/plugin/oracle.pp
manifests/plugin/ovs_events.pp
manifests/plugin/bind.pp
manifests/plugin/rabbitmq.pp
manifests/plugin/iptables.pp
manifests/plugin/mcelog.pp
manifests/plugin/write_http.pp
manifests/plugin/perl/plugin.pp
manifests/plugin/write_sensu.pp
manifests/plugin/virt.pp
manifests/plugin/nginx.pp

There are small differences in the various files, for example manifests/plugin/python.pp tests a bit more, maybe it's worth having this logic everywhere if it apply likewise to other modukes?

smortex avatar Sep 16 '21 18:09 smortex

The thing is that the package for the plugin is being called by each of those classes, which are the classes you declare in your profile to add the plugin. And I don't think it's straightforward to move the package resource into the generic plugin.pp class as the package name might not always match collectd-. In my opinion the quickest win at this stage is for people to add this test in the plugins where they encounter this issue.

markasammut avatar Sep 17 '21 08:09 markasammut

@smortex any chance this could be merged in please?

markasammut avatar Dec 03 '21 09:12 markasammut

@markasammut can you rebase and squash your commits?

kenyon avatar Dec 03 '21 19:12 kenyon

@kenyon rebased. Can't you squash when merging the PR with a Squash&Merge? Or please let me know how can I do it on an open PR.

markasammut avatar Dec 09 '21 09:12 markasammut

From your working directory:

git rebase -i origin/master # Interactively rewrite history

This will bring your editor. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will allow you to combine the commit messages in a single commit message.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

smortex avatar Dec 09 '21 17:12 smortex

Rebased and Squashed as instructed. Thanks!

markasammut avatar Dec 28 '21 14:12 markasammut

@smortex any plan to merge this? Had to add other fixes to my fork (with respect to the ValuesFrom parameter in the query.conf of the postgresql plugin)

markasammut avatar Jan 19 '22 16:01 markasammut

Hey!

Please submit each change proposal in a different PR :-). The easiest way is to create feature branches: one new branch per feature / bug fix. When a branch is updated, the corresponding PR show the changes, so now this PR include your ValuesFrom work which is unrelated.

My recommendations:

  1. Create a new branch values_from from your current master branch;
  2. Remove the commits related to ValuesFrom in your master branch (with git rebase -i as indicated in my previous comment, just follow the instructions in the editor)
  3. Force push to fix this PR
  4. Remove the commits related to this PR from the values_from branch
  5. Push this branch to your fork and open a PR for this other change if it is something you want to merge

Thanks!

smortex avatar Jan 19 '22 17:01 smortex

Yeah those last commits need removing before this can be merged.

Going to add the tag needs-work to this. Please remove the tag when that is done.

Steve

traylenator avatar Sep 02 '22 13:09 traylenator

This has been replaced by two separate PRs as requested:

  • https://github.com/voxpupuli/puppet-collectd/pull/1009
  • https://github.com/voxpupuli/puppet-collectd/pull/1010

markasammut avatar Dec 27 '22 14:12 markasammut