Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugin: update cpu for collectd 5.8 #801

Merged
merged 1 commit into from May 31, 2018

Conversation

sileht
Copy link
Contributor

@sileht sileht commented May 25, 2018

This change adds cpu plugin options added in collectd 5.8

@@ -5,6 +5,8 @@
Boolean $reportbycpu = true,
Boolean $valuespercentage = false,
Boolean $reportnumcpu = false,
Boolean $reportgueststate = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datatypes \o/

@@ -6,5 +6,9 @@
<% if scope.lookupvar('collectd::collectd_version_real') and scope.function_versioncmp([scope.lookupvar('collectd::collectd_version_real'), '5.6']) >= 0 -%>
ReportNumCpu <%= @reportnumcpu %>
<% end -%>
<% if scope.lookupvar('collectd::collectd_version_real') and scope.function_versioncmp([scope.lookupvar('collectd::collectd_version_real'), '5.8']) >= 0 -%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if @collectd::collectd_version_real should work as well. scope.lookupvar() is legacy stuff to be compatible with very old puppet versions. It isn't needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why this is even needed, I have checked how this variable is built, and if the fact return null or undef, we pick the minimal_version. So it can never be null or I miss something ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure if that's still needed. That's something we could check and cleanup.

<% if scope.lookupvar('collectd::collectd_version_real') and scope.function_versioncmp([scope.lookupvar('collectd::collectd_version_real'), '5.8']) >= 0 -%>
ReportGuestState <%= @reportgueststate %>
SubtractGuestState <%= @subtractgueststate %>
<% end -%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you interested in converting this erb template into epp? This is not required, but we would be very happy about it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK, the file small enought, I done it :)

@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels May 30, 2018
@bastelfreak
Copy link
Member

Thanks for the PR @sileht ! I added some inline comments.

This change adds options for cpu plugin present since collectd 5.8
@bastelfreak bastelfreak removed the needs-work not ready to merge just yet label May 31, 2018
@bastelfreak bastelfreak merged commit 3951f1f into voxpupuli:master May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants