-
Notifications
You must be signed in to change notification settings - Fork 26
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
Skip versioncmp when pe_server_version is missing #23
Conversation
@@ -18,7 +18,7 @@ | |||
override_metrics_command => $override_metrics_command, | |||
} | |||
|
|||
if versioncmp($facts['pe_server_version'], '2018.1.0') < 0 { | |||
if ($facts['pe_server_version'] =~ NotUndef) and (versioncmp($facts['pe_server_version'], '2018.1.0') < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add a parameter that would allow users to enable these if they are on a new enough version of puppetserver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be enabled for open source Puppet Server 5.x or newer. I promoted these metrics to the /status
API in that version. The pe_server_version
check here is testing for older versions of PE and uses the /metrics
API to retrieve the data there.
Most Puppet Server metrics were "PE only" prior to 5.x, so there isn't much to gain by backporting this specific check, but we should think about how to do version-specific metrics in a way that supports both PE and Open Source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit updates the version checking logic in puppet_metrics_collector::puppetserver to skip `versioncmp()` when the `pe_server_version` fact is missing. This change allows the class to be applied to FOSS nodes without causing a compilation failure.
790f89a
to
10b58fb
Compare
@npwalker Are you alright with merging this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit updates the version checking logic in
puppet_metrics_collector::puppetserver to skip
versioncmp()
when thepe_server_version
fact is missing. This change allows the class tobe applied to FOSS nodes without causing a compilation failure.