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

Validate Alertmanager config #277

Merged
merged 13 commits into from
May 30, 2019
Merged

Validate Alertmanager config #277

merged 13 commits into from
May 30, 2019

Conversation

allangood
Copy link

Install the amtool and validate alertmanager config file prior changes.

Pull Request (PR) description

Validate Alertmanager config file prior changes

This Pull Request (PR) fixes the following issues

Alertmanager has a validation tool but it's not installed nor used by this modules until now.

Install the amtool and validate alertmanager config file prior changes.
Fixed lint and require parameter
@allangood
Copy link
Author

Fixed require parameter and cleaned all lint warnings.

Uses bin_dir instead of prometheus::server::bin_dir
content => template('prometheus/alertmanager.yaml.erb'),
notify => $notify_service,
require => File["${bin_dir}/amtool", $config_dir],
validate_cmd => "${bin_dir}/amtool check-config --alertmanager.url='' %",
Copy link
Member

Choose a reason for hiding this comment

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

do you know if this works for different versions of the alertmanager? Can you provide an acceptance test for this?

Copy link
Author

Choose a reason for hiding this comment

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

Doing a quick check on Alertmanager Github page, the amtool command was implemented in version 0.10.0. I could implement a "versioncmp" to make sure to do the validation check if the version is equal or grater than 0.10.0. This works for you?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good!

Copy link
Author

@allangood allangood left a comment

Choose a reason for hiding this comment

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

I just realized that I've introduced an old behaviour in the "notify" parameter. Let me correct it.

@allangood
Copy link
Author

Nothing left from my part.

@bastelfreak
Copy link
Member

@allangood can you add an acceptance test for this?

@allangood
Copy link
Author

allangood commented Nov 2, 2018

Never did it before, I will do my best using "prometheus_server_spec.rb" as template.

@bastelfreak
Copy link
Member

Ping @allangood :)

@allangood
Copy link
Author

Sorry about this long silence... I started to work with something else and completely forgot to commit this code.
Sorry again.

Acceptance test for Alertmanager
Alertmanager acceptance test via class
@allangood
Copy link
Author

Well, looks like I need some guidance with the acceptance test! Someone could give me a hand?


describe 'prometheus alertmanager' do
it 'alertmanager works idempotently with no errors' do
pp = "class { 'prometheus::alertmanager': }"
Copy link
Member

Choose a reason for hiding this comment

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

the previous style was correct, you can undo the last commit.

@bastelfreak
Copy link
Member

Hi. The test itself works. Puppet creates the service alertmanager, it's also enabled. It fails to start during the first puppet run and also during the second. that's why there isn't an open TCP port. Did you test this locally? I assume the generated config isn't valid or something is missing.

@bastelfreak bastelfreak added enhancement New feature or request tests-fail labels May 26, 2019
@bastelfreak
Copy link
Member

Thanks for all the work @allangood

@bastelfreak bastelfreak merged commit 8925168 into voxpupuli:master May 30, 2019
@allangood
Copy link
Author

Wow! It's working now! I had to include some configuration to Alertmanager, the default "include prometheus::alermanager" is not enough.

Sorry for the amount of commits. :)

@allangood allangood deleted the patch-4 branch May 30, 2019 15:42
cegeka-jenkins pushed a commit to cegeka/puppet-prometheus that referenced this pull request Aug 28, 2019
* Validate Alertmanager config

Install the amtool and validate alertmanager config file prior changes.

* Fixed lint and require parameter

Fixed lint and require parameter

* Removed trailing whitespace

* Uses bind_dir

Uses bin_dir instead of prometheus::server::bin_dir

* Update alertmanager.pp

Check Alertmanager version.

* Identation corrected

* Corrected notify service

* Acceptance test for Alertmanager

Acceptance test for Alertmanager

* Alertmanager acceptance test via class

Alertmanager acceptance test via class

* Using my tested config for Alertmanager

* Minimal configuration for Alertmanager

* Fixed syntax, added missing closing braces

* Removed IPv4 configuration to allow IPv6 test
Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021
* Validate Alertmanager config

Install the amtool and validate alertmanager config file prior changes.

* Fixed lint and require parameter

Fixed lint and require parameter

* Removed trailing whitespace

* Uses bind_dir

Uses bin_dir instead of prometheus::server::bin_dir

* Update alertmanager.pp

Check Alertmanager version.

* Identation corrected

* Corrected notify service

* Acceptance test for Alertmanager

Acceptance test for Alertmanager

* Alertmanager acceptance test via class

Alertmanager acceptance test via class

* Using my tested config for Alertmanager

* Minimal configuration for Alertmanager

* Fixed syntax, added missing closing braces

* Removed IPv4 configuration to allow IPv6 test
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.

3 participants