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

add explicit parameter for retention #137

Merged

Conversation

costela
Copy link

@costela costela commented Jan 3, 2018

This way we don't need to use $extra_options. Should make module slightly more newbie-friendly.

Fixes #136

@costela costela force-pushed the make_retention_configurable branch 3 times, most recently from 5ca7c2e to 061061f Compare January 3, 2018 16:09
@@ -7,6 +7,7 @@
$remote_read_configs,
$purge = true,
$config_template = $::prometheus::params::config_template,
$retention = $::prometheus::params::storage_retention,
Copy link
Member

Choose a reason for hiding this comment

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

You are referencing the variable from params.pp. Why not the variable from init.pp?
In your case the value is always fix and not changeable.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @tuxmea here

Copy link
Author

Choose a reason for hiding this comment

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

maybe I misunderstood your comment, but the parameter is changeable because it gets passed inside init.pp, in the instantiation of class[::prometheus::config] (and it's also a parameter to init itself).
and I wanted to stick to the current coding style, since $config_template is implemented in exactly the same way (AFAICT)

Copy link
Member

Choose a reason for hiding this comment

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

@costela ok. I missed the parameter hand over in init.pp
Technically OK. Not my most favorite solution, but working as expected.

Copy link
Author

Choose a reason for hiding this comment

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

oh, BTW, time for a bit of bikeshedding: retention or storage_retention? I couldn't decide ... 😁

@@ -151,6 +151,7 @@
$alerts = $::prometheus::params::alerts,
Array $alert_relabel_config = $::prometheus::params::alert_relabel_config,
Array $alertmanagers_config = $::prometheus::params::alertmanagers_config,
String $retention = $::prometheus::params::storage_retention,
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/

Copy link
Author

Choose a reason for hiding this comment

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

is this a request for a more specific datatype or just celebrating the current String datatype? 😄
I mean, I could use a regexp type, but it felt a bit overkill...

this way we don't need to use $extra_options
@costela costela force-pushed the make_retention_configurable branch from 061061f to 346e028 Compare January 4, 2018 10:53
@@ -151,6 +156,7 @@
$alerts = $::prometheus::params::alerts,
Array $alert_relabel_config = $::prometheus::params::alert_relabel_config,
Array $alertmanagers_config = $::prometheus::params::alertmanagers_config,
String $storage_retention = $::prometheus::params::storage_retention,
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/

@bastelfreak bastelfreak added the enhancement New feature or request label Jan 4, 2018
@bastelfreak
Copy link
Member

Thanks for the PR @costela !

@bastelfreak bastelfreak merged commit 5a6fa9d into voxpupuli:master Jan 4, 2018
@costela costela deleted the make_retention_configurable branch January 4, 2018 13:53
cegeka-jenkins pushed a commit to cegeka/puppet-prometheus that referenced this pull request Aug 28, 2019
…igurable

add explicit parameter for retention
Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021
…igurable

add explicit parameter for retention
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.

4 participants