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

node_exporter 0.15.0 compatibiity #72

Merged
merged 3 commits into from
Oct 19, 2017
Merged

node_exporter 0.15.0 compatibiity #72

merged 3 commits into from
Oct 19, 2017

Conversation

TheMeier
Copy link
Contributor

fixes #70
fixes #33

change collector parameterization to be compatible with node_exporter 0.15.0

this should be released with a major version since it breaks paramter compatinbility

for two step migration of profiles and roles using this keep node_exporter::collectors paramter as noop with deprecation warning for at least one release

$service_name = 'node_exporter',
$user = $::prometheus::params::node_exporter_user,
$version = $::prometheus::params::node_exporter_version,
$arch = $::prometheus::params::arch,
Copy link
Member

Choose a reason for hiding this comment

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

please add datatypes to those parameters. An example can be found at https://github.com/voxpupuli/puppet-prometheus/pull/68/files

Copy link
Contributor Author

@TheMeier TheMeier Oct 18, 2017

Choose a reason for hiding this comment

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

hm isn't adding datatypes to all params a different aspect that should be adressed in a seperate PR?

well i did it anyway :)

@@ -113,19 +124,28 @@
validate_bool($manage_user)
Copy link
Member

Choose a reason for hiding this comment

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

please drop those validate_* calls. They are not needed if we have datatypes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -113,19 +124,28 @@
validate_bool($manage_user)
validate_bool($manage_service)
validate_bool($restart_on_change)
validate_array($collectors)
if $collectors != undef {
Copy link
Member

Choose a reason for hiding this comment

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

this is enough:

if $collectors  { }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@bastelfreak
Copy link
Member

Thanks for this PR @TheMeier!

  • I added a few inline comments, can you please review them?
  • Please check your used email address, it isn't associated with your github account

@bastelfreak
Copy link
Member

Hi @TheMeier, thanks for the updates. Can you check the used email address in your first commit and add it to your github profile? Otherwise the commti isn't associated with you (or change the email address in the commit).

fixes #70
fixes #33

change collector parameterization to be compatible with node_exporter 0.15.0

this should be released with a major version since it breaks paramter compatinbility

for two step migration of profiles and roles using this keep node_exporter::collectors paramter as noop with deprecation warning for at least one release
@TheMeier
Copy link
Contributor Author

email should be fine now

@bastelfreak
Copy link
Member

Thanks!

@bastelfreak bastelfreak merged commit b6f4246 into voxpupuli:master Oct 19, 2017
cegeka-jenkins pushed a commit to cegeka/puppet-prometheus that referenced this pull request Aug 28, 2019
node_exporter 0.15.0 compatibiity
Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021
node_exporter 0.15.0 compatibiity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node_expoerter v0.15.0 Default Node Exporter Collectors
3 participants