-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
nixos/prometheus: check alertmanager configuration #49756
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
Conversation
I gave it a quick look and it seems that the code does not require any resources not available in the sandbox: https://github.com/prometheus/alertmanager/blob/6d0edbe63066b4d0b7c189afb8e6849458455f65/cli/check_config.go |
@GrahamcOfBorg test prometheus |
Timed out, unknown build status on x86_64-linux (full log) Attempted: tests.prometheus Partial log (click to expand)
|
Success on aarch64-linux (full log) Attempted: tests.prometheus Partial log (click to expand)
|
I updated the PR to contain the test for alertmanager. Since the empty alertmanager is not valid according to cc @WilliButz |
This commit adds an assertion that checks that either `configFile` or `configuration` is configured for alertmanager. The alertmanager config can not be an empty attributeset. The check executed with `amtool` fails before the service even has the chance to start. We should probably not allow a broken alertmanager configuration anyway. This also introduces a test for alertmanager configuration that piggy backs on the existing prometheus tests.
Updated the assertion as @WilliButz proposed. @GrahamcOfBorg test prometheus |
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: tests.prometheus Partial log (click to expand)
|
No attempt on x86_64-linux (full log) The following builds were skipped because they don't evaluate on x86_64-linux: tests.prometheus Partial log (click to expand)
|
It seems like ofborg is (still) having issues. I did re-run the tests locally and they passed. Merging this. |
Way cool! |
Motivation for this change
This is the same as #49749 just for alertmanager.
cc @ocharles @fpletz @bjornfor @teh @globin
Still running tests for this. Might even end up adding an alertmanager test.