-
-
Notifications
You must be signed in to change notification settings - Fork 239
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 validation to config changes #122
Conversation
255b121
to
377b264
Compare
@costela Many thanks for the PR. Please check the travis error: |
@tuxmea yeah, I'm on it. Had a last minute change of heart on the last patch, and it didn't go very smoothly 😉 |
bec0510
to
2a8b1b1
Compare
@tuxmea done |
Please note there's a small breaking change in the last patch: the |
class prometheus::install | ||
{ | ||
class prometheus::install ( | ||
Boolean $purge_config_dir = true, |
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.
Datatypes \o/
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.
Is there a specific reason why config_dir was moved from config.pp to install.pp?
@@ -78,4 +82,11 @@ | |||
system => true, | |||
}) | |||
} | |||
file { $prometheus::config_dir: |
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.
why moving this from config to install class?
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.
as I mentioned in a comment above: this was necessary to add the missing dependency between the config file and the alerts file without creating a dependency cycle. Now both depend on the directory, which gets created first during install.
The config file needs the alerts file, because the newly introduced validation with promtool
chokes if it can't find the named alerts file.
That's also the reason why I removed the conditional creation of the alerts file and create it always (even if empty). Otherwise the dependency would have to be conditional on the alert file's creation and the prometheus.yaml.erb
template would also have to be changed to not always reference a possibly non-existent alerts file.
In summary: it just felt like the path of least resistance 😉
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.
ACK +1
This is needed to break a dependency cycle involving alert.rules and the main config. Minor breaking change: if anyone used the purge argument directly to prometheus::config (instead of the prometheus class itself), the call will have to be adjusted.
Add validation to config changes
This PR adds a symlink to promtool in the same way a symlink to prometheus is currently added to /usr/local/bin.
Than it uses File's
validate_cmd
to call promtool and avoid replacing a configuration file with a broken version, both for the central config and for the alert rules.Finally, also fix the first error thrown by the config check: we create the alerts file even if no alerts were provided. The empty file won't trigger a syntax error and also doesn't bother prometheus itself.
Fixes #31