logstash: config,patternfile respect $::logstash::ensure#411
logstash: config,patternfile respect $::logstash::ensure#411damonbreeden wants to merge 3 commits intovoxpupuli:masterfrom
$::logstash::ensure#411Conversation
otherwise if a server has `ensure => false` but still calls `logstash::configfile` for any reason the file will try to be created but fail bc `/etc/logstash` does not exist
|
ping 🙏 |
smortex
left a comment
smortex
left a comment
There was a problem hiding this comment.
That… looks… reasonably… safe… ? 🙄
/me loves CI
|
Wait, we do have CI in this project. I try to trigger it by closing and re-opening it… |
manifests/configfile.pp
Outdated
| # @author https://github.com/elastic/puppet-logstash/graphs/contributors | ||
| # | ||
| define logstash::configfile ( | ||
| $ensure = $::logstash::ensure, |
There was a problem hiding this comment.
If you think this parameter is required, please add some documentation above:
# @param ensure
# ...
But AFAIAC, i think we do not need this extra parameter and we can directly use logstash::ensure. In this case, this line can be removed, and the two bellow changes are required:
There was a problem hiding this comment.
i think that more granularity (with proper default values) is always better
in this case, we had a configfile that was placed by this module, then later i wanted to remove it so i made these changes and specifically called absent
could i have accomplished the same with a file resource? yes ofc! but we place the files in /etc/logstash/conf.d with logstash::configfile so we should be able to remove them the same way
alternatively (or probably?) we could/should manage the entire /etc/logstash/conf.d directory (with purge => true so that no files are leftover
|
|
||
| if($config) { | ||
| file { $config_file: | ||
| ensure => $ensure, |
There was a problem hiding this comment.
| ensure => $ensure, | |
| ensure => $logstash::ensure, |
There was a problem hiding this comment.
if there's a $ensure parameter, we should also use it?
| } | ||
| elsif($source) { | ||
| file { $config_file: | ||
| ensure => $ensure, |
There was a problem hiding this comment.
| ensure => $ensure, | |
| ensure => $logstash::ensure, |
| # @author https://github.com/elastic/puppet-logstash/graphs/contributors | ||
| # | ||
| define logstash::patternfile ( | ||
| $ensure = $::logstash::ensure, |
There was a problem hiding this comment.
Same, either we need to document this…
|
|
||
| file { "${logstash::config_dir}/patterns/${destination}": | ||
| ensure => file, | ||
| ensure => $ensure, |
There was a problem hiding this comment.
…otherwise:
| ensure => $ensure, | |
| ensure => $logstash::ensure, |
|
@damonbreeden please rebase against our latest master branch to get rid of the merge commit. |
| # | ||
| # Parameters are mutually exclusive. Only one should be specified. | ||
| # | ||
| # @param [String] ensure |
There was a problem hiding this comment.
please don't list the datatype, that's a legacy notation and not required anymore. puppet-strings will parse it out of the code now.
| # @param [String] ensure | |
| # @param ensure |
3 participants
otherwise if a server has
ensure => falsebut still callslogstash::configfilefor any reason the file will try to be created but fail bc/etc/logstashdoes not existe.g. in your private
base.ppyou saybut somewhere in hiera you don't want to install
logstashon a cluster so you havelogstash::ensure: absent