-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
amtool check-config #978
amtool check-config #978
Conversation
c90f7cf
to
ab677d6
Compare
@iksaif Thanks a lot for the contribution. This was requested very often. I am not familiar with the amtool testing story (//CC @stuartnelson3), but I think we should have a basic test. @iksaif would you be able to add one for the configuration check? |
Ah I forgot to comment! I tested this manually this morning, worked great. A really high level smoke test should be enough if it's not too much trouble -- does it recognize a good config.yml and an bad one. |
Added tests |
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.
Thanks for the test. Just some small comments below. Other than that this looks good to me. @stuartnelson3 please merge if you feel the same.
cli/check_config_test.go
Outdated
|
||
err = CheckConfig([]string{"testdata/conf.bad.yml"}) | ||
if err == nil { | ||
t.Fatalf("Failed to detect missing file") |
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.
I am confused by the word missing
here. How about failed to detect invalid file
?
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.
Forgot to change this when I changed the test
cli/check_config_test.go
Outdated
func TestCheckConfig(t *testing.T) { | ||
err := CheckConfig([]string{"testdata/conf.good.yml"}) | ||
if err != nil { | ||
t.Fatalf("error: %v", err) |
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.
Instead of error:
how about checking valid config file failed with:
?
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.
done
couple small wording changes on the test failure output and then we'll merge |
This is similar to `promtool check-config` and allows one to validate the alertmanager configuration (as a git presubmit for example). `govendor fetch github.com/spf13/{cobra,pflag}` was needed to have support for `Args`.
done |
This is similar to `promtool check-config` and allows one to validate the alertmanager configuration (as a git presubmit for example). `govendor fetch github.com/spf13/{cobra,pflag}` was needed to have support for `Args`.
This is similar to `promtool check-config` and allows one to validate the alertmanager configuration (as a git presubmit for example). `govendor fetch github.com/spf13/{cobra,pflag}` was needed to have support for `Args`.
* Replace supervisord xmlrpc library * Use `github.com/mattn/go-xmlrpc` that doesn't leak goroutines. * Fix uptime metric * Use Prometheus best practices for uptime metric. * Use "start time" rather than "uptime". * Don't emit a start time if the process is down. * Add changelog entry. * Add example compatibility rules. Signed-off-by: Ben Kochie <[email protected]>
This is similar to
promtool check-config
and allows oneto validate the alertmanager configuration (as a git presubmit for example).
govendor fetch github.com/spf13/{cobra,pflag}
was needed tohave support for
Args
.This fixes #333