amtool: Detect version drift and warn users#2672
amtool: Detect version drift and warn users#2672roidelapluie merged 2 commits intoprometheus:mainfrom
Conversation
This change detects the alertmanager version when initiating the client. It ignores most errors since I expect amtool to fail later. If amtool is not compiled with proper version, we do not do anything either. We use MajorMinor for now as we have not reach 1.0, but we still allow the bugfix version number (Z in x.y.Z) to differ. Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
gotjosh
left a comment
gotjosh
left a comment
There was a problem hiding this comment.
Approach LGTM, some quick questions on whenever we should have a way to make this optional.
|
|
||
| return client.New(cr, strfmt.Default) | ||
| c := client.New(cr, strfmt.Default) | ||
|
|
There was a problem hiding this comment.
Given this is now executed for every command, can we an option to skip it?
Something like:
var skipVersionCheck bool
app.Flag("skip.version-check", "Skip version checks when executing commands.").BoolVar(&skipVersionCheck)
// Then, we can do something like
if skipVersionCheck {
return c
}|
|
||
| status, err := c.General.GetStatus(nil) | ||
| if err != nil || status.Payload.VersionInfo == nil || version.Version == "" { | ||
| // We can not get version info, or we do not know our own version. Let amtool continue. |
There was a problem hiding this comment.
Now that we have an option to cancel the check, I don't think we should fail silently. How do you feel about logging that we were unable to compute the version and tell the user that should upgrade/downgrade?
There was a problem hiding this comment.
If we can not run the check, it is because alertmanager does not work at all, and the error will be printed in the actual call later on.
There was a problem hiding this comment.
I was thinking more in the case of Grafana but this is not a concern for us here to be honest.
| return c | ||
| } | ||
|
|
||
| if semver.MajorMinor("v"+*status.Payload.VersionInfo.Version) != semver.MajorMinor("v"+version.Version) { |
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
|
updated |
There was a problem hiding this comment.
LGTM.
$ ./amtool --alertmanager.url=http://localhost:9093 config show
Warning: amtool version (0.22.2) and alertmanager version (0.21.0) are different.
# and then
$ ./amtool --alertmanager.url=http://localhost:9093 config show --no-version-checkBoth work as expected.
2 participants
This change detects the alertmanager version when initiating the client.
It ignores most errors since I expect amtool to fail later.
If amtool is not compiled with proper version, we do not do anything
either.
We use MajorMinor for now as we have not reach 1.0, but we still allow
the bugfix version number (Z in x.y.Z) to differ.
Signed-off-by: Julien Pivotto roidelapluie@inuits.eu