voxpupuli / puppet-collectd

Collectd module for Puppet
https://forge.puppet.com/puppet/collectd
Apache License 2.0
69 stars 272 forks source link

Ensure plugin version is same version as main collectd package #986

Closed markasammut closed 1 year ago

markasammut commented 3 years ago

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.

smortex commented 3 years ago

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?

markasammut commented 3 years ago

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 commented 2 years ago

@smortex any chance this could be merged in please?

kenyon commented 2 years ago

@markasammut can you rebase and squash your commits?

markasammut commented 2 years ago

@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.

smortex commented 2 years ago

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)
markasammut commented 2 years ago

Rebased and Squashed as instructed. Thanks!

markasammut commented 2 years ago

@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)

smortex commented 2 years ago

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!

traylenator commented 2 years ago

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

markasammut commented 1 year ago

This has been replaced by two separate PRs as requested: