Add install_options for RedHat Family in sensors plugin #664
Conversation
| ) { | ||
|
|
||
| include ::collectd | ||
|
|
||
| $_manage_package = pick($manage_package, $::collectd::manage_package) | ||
|
|
||
| if $package_install_options != undef { | ||
| validate_array($package_install_options) |
There was a problem hiding this comment.
instead of this validation block, could you update package_install_options in the params.pp so it is an optional array?
There was a problem hiding this comment.
I copied the code from the apache.pp file, for all occurances. Is there any plan on you guys refactoring the entire code ? Actually none of the RedHat occurences uses the correct options.
Will see what and how I can fix this.
There was a problem hiding this comment.
It is best practice to define the array with all arguments. You don't know the amount of options used however.
Optional[Array[String[]]] $package_install_options = undef
will allow only any number of elements in the array.
@bastelfreak Can you please advise if this is your prefered choice ?It does not seem to be the way to go according to several sources.
There was a problem hiding this comment.
This should work:
Optional[Array[String]] $package_install_options = undefour time is limited, we will refactore the whole code somewhere in the future. We would like to already use the new datatypes for all new pull requests.
| if $::osfamily == 'Redhat' { | ||
| if $_manage_package { | ||
| package { 'collectd-sensors': | ||
| ensure => $ensure, | ||
| ensure => $ensure, | ||
| install_options => $package_install_options, |
There was a problem hiding this comment.
can you add spec tests for this?
|
Hey @ziporah, thanks for your contribution! Can you please take a look at the used email address in your commit? It isn't asociated with your github account. Can you please add the address or change it? |
|
Puppet 4 introduced nice datatypes, this allows us to get rid of all the validate_* functions. They are now deprecated. Can you replace the validate call with the proper datatype? Feel free to join our IRC channel #voxpupuli on freenode if you've any questions. We're almost 24/7 reachable. |
|
Ping @ziporah |
|
sorry, have been busy with some major upgrades. I'm trying to pick this up again in august |
|
@ziporah do you still want to rework this? |
|
We are planning to take this on in a refactoring round the upcoming months.
If you want someone else to fill this in, please do so. Otherwise expect the reworked code in the next months.
Van: Will Yardley [mailto:[email protected]]
Verzonden: vrijdag 15 september 2017 7:45
Aan: voxpupuli/puppet-collectd <[email protected]>
CC: ziporah <[email protected]>; Mention <[email protected]>
Onderwerp: Re: [voxpupuli/puppet-collectd] Add install_options for RedHat Family in sensors plugin (#664)
@ziporah <https://github.com/ziporah> do you still want to rework this?
It looks like it will need a rebase now also.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#664 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/ADmV6SnwbW5LM1BwIQ_--M_8jgYmQyRfks5sig7cgaJpZM4NBmnI> . <https://github.com/notifications/beacon/ADmV6SvgG8McJtIovE0ZkoFQYNcEYwMWks5sig7cgaJpZM4NBmnI.gif>
|
|
HI @ziporah, are you still interested in this? Otherwise we're going to close this next month. |
|
Hey @bastelfreak my collegue will create a new PR this week that should work as per your request. I'm currently to busy. |
|
I'm going to close this since we merged #762 |
This patch adds the global specified installation options for the sensors plugin