Switch amtool to kingpin by carlpett · Pull Request #976 · prometheus/alertmanager

carlpett · 2017-09-04T12:55:20Z

Related to #974, this PR switches amtool to use kingpin.

This PR is a bit more involved, since cobra/viper does not translate quite as easily as the core flag package. There are some remaining points before this can be merged that I would like feedback on:

There were quite nice long format help texts written previously. There is no "long help" field in kingpin, but maybe it would be a good idea to make the man page template (--help-man) use it?
Is the config file an important feature? Kingpin does not have any file loading capabilities, so we'll need to built it ourselves in that case, and ensure it falls back in the correct order. If so, maybe the number of file formats could be reduced to avoid all too much complexity?
What is the purpose of date.format? It is only exposed through the config file from what I understand of the code, and it is not documented.
comment-required is not implemented since it is only supported through config-file at the moment.

Let me know if there are any other changes you think are necessary! @stuartnelson3 @fabx

stuartnelson3 · 2017-09-05T16:27:46Z

👍
It would be good to maintain the config file functionality, since there is a lot of additional metadata that regularly needs to be added since the cli tool does affect alertmanager. I would say stick with .yml and support the two location that are currently supported.
Not sure .. @Kellel ? I've never used it, and I think we're limited by the date format that alertmanager parses.
I think it would be best to require a comment, since we require one through the UI.

Thanks for doing this!

Kellel · 2017-09-05T17:19:03Z

The date.format option is only used for output formatting. I could see people using it, but it just a preference option. I mean, I even forgot about it and I am the one who wrote it.

Kellel · 2017-09-05T17:52:35Z

I am happy with the comment being required, but if so we should enforce this at the api level as well (Out of scope for this PR).

Kellel · 2017-09-05T18:03:52Z

Also as far as the config file goes, viper (the config file library) can be used without cobra (the flag parsing library). So you can feasibly still use viper for all the config file processing and kingpin for the flags. I don't know if that will be easier, but it might be worth exploring.

carlpett · 2017-09-05T19:01:14Z

Thanks for the input!

@stuartnelson3 Sounds good. I'll implement some config file loading.

@Kellel Regarding using viper, I looked into that a bit, but from what I can see in the related issue, it doesn't look trivial at least.

carlpett · 2017-12-17T18:26:33Z

So, I found this PR lying around on my laptop, thought I had finished it long ago... Sorry about that. I finished up the small work that remained (which mainly consisted of getting a PR into kingpin, done since many months now), and now I'm working through the many merge conflicts... Looks like it could take a while longer than I have available today, but I hope to finish it tomorrow.

stuartnelson3 · 2017-12-18T09:19:33Z

cool, thanks for the update!

carlpett · 2017-12-19T17:08:40Z

So, merging old branches is a pain (no news there). I'm testing it a bit manually now to make sure parsing etc does what it should, but I'm also seeing the acceptance tests fail. I don't understand why, though, as far as I see, amtool is not used for this?

carlpett · 2017-12-19T21:56:04Z

Ok, acceptance tests are suddenly green again after some last fixes of other things. Are they known to be flakey, or could I have broken something?

Anyway, I think I've addressed all the points I had in the original post:

Long-form help-texts are given with --help-long.
The config file support is implemented (although only in the default locations, and only with yaml format)
date.format has been given a flag, as well as works from config file
require-comment is supported in the config file, and has a (hidden) flag as well

Let me know what you think!

stuartnelson3 · 2017-12-20T09:44:05Z

Taking a look, thanks!

And yeah, the acceptance tests are flakey :/

stuartnelson3

mostly superficial changes, but otherwise looks good. will run this locally to see how it works.

stuartnelson3 · 2017-12-20T09:48:02Z

cli/alert.go

 	"strings"
 	"time"

+	"github.com/alecthomas/kingpin"


We should use gopkg.in/alecthomas/kingpin.v2, like in prometheus (and as recommended in the repo)

Actually, no :) We need to use the v3-branch, since the pull request I made to kingpin to add support for config files is only present on that branch.

stuartnelson3 · 2017-12-20T09:50:32Z

cli/alert.go

-	`,
-	Run: CommandWrapper(queryAlerts),
-}
+	If alertname is ommited and the first argument does not contain a '=' or a


/s/ommitted/omitted/

stuartnelson3 · 2017-12-20T09:54:20Z

cli/config.go

-	"path"
 	"time"

+	"github.com/alecthomas/kingpin"


same import issue as above

stuartnelson3 · 2017-12-20T09:56:32Z

cli/format/format.go

 	"io"
 	"time"

+	"github.com/alecthomas/kingpin"


stuartnelson3 · 2017-12-20T09:59:30Z

cli/root.go

-from one of two default config locations. Valid config file formats are JSON, TOML, YAML, HCL and Java Properties, use
-whatever makes sense for your project.
+// This function maps things which have previously had different names in the
+// config file to their new names, so old configrations keep working


configurations

stuartnelson3 · 2017-12-20T10:02:37Z

cli/root.go

+		Vars:     map[string]interface{}{"LongHelp": longHelpText},
+	}).Bool()
+
+	configResolver := newConfigResolver()


I would prefer to have newConfigResolver return an error, and instead of panicking on a file read error or a yaml unmarshal error, provide a shorter and clearer message. panic()'s stack trace probably won't aid a user much, and generate a github issue when they just need to fix their yaml.

stuartnelson3 · 2017-12-20T10:04:16Z

cli/silence_add.go

-	"path"
 	"time"

+	"github.com/alecthomas/kingpin"


stuartnelson3 · 2017-12-20T10:29:09Z

cli/silence_expire.go

+	basePath := "/api/v1/silence"
+	for _, id := range *expireIds {
+		u := GetAlertmanagerURL(path.Join(basePath, id))
 		req, err := http.NewRequest("DELETE", u.String(), nil)


again, not your fault, but could you do the if err != nil dance here?

carlpett · 2017-12-20T12:35:34Z

@stuartnelson3 Pushed fixes for all the comments except the import path, because of the reason I replied with there. I think there might be a bit of time until v3 is released, given the number of remaining items on the roadmap, but I haven't had any stability issues in our usage of "v3-unstable" in our work projects.

stuartnelson3 · 2017-12-21T12:10:55Z

Looks great, works great. I have one final, tiny request (or question, I guess), and then this is merged. You mentioned that extended help is available via --help-long, and that's the same flag that's mentioned in the kingpin documentation. It's implemented here as --long-help. I'm fine with either since it shows up as a flag when a user does -h, but just wanted to see if this was done on purpose or not.

carlpett · 2017-12-21T12:59:29Z

Oh, uh, that is just a silly mistake on my part... I don't know how I miswrote that. Pushing in a sec!

stuartnelson3 · 2017-12-22T10:16:45Z

No worries :)

stuartnelson3

Thanks for this!

Possibly another regression introduced by prometheus#976 . We use the wrong variable to update comments in the `amtool silence update` command which causes us to fail silently. This fixes that.

Possibly another regression introduced by #976 . We use the wrong variable to update comments in the `amtool silence update` command which causes us to fail silently. This fixes that.

josedonizetti mentioned this pull request Oct 9, 2017

Refactoring: Extract alertmanager http cli from amtool #1035

Closed

carlpett force-pushed the kingpin-amtool branch from c30617b to 6f7279c Compare October 13, 2017 13:19

carlpett added 5 commits December 19, 2017 19:39

Switch cmd/amtool to kingpin

6f7d047

Touch-ups

d98a05e

Implement long help

c4706f0

Add missing short-form of --output

3431d21

Fix backwards compatibility for config file options

e8190b9

carlpett force-pushed the kingpin-amtool branch from 056c8d9 to e8190b9 Compare December 19, 2017 21:51

Fix vendoring

09a2e74

carlpett changed the title ~~WIP: Switch amtool to kingpin~~ Switch amtool to kingpin Dec 19, 2017

stuartnelson3 suggested changes Dec 20, 2017

View reviewed changes

Review fixes

4efe840

Fix flag word order

3848aae

stuartnelson3 approved these changes Dec 22, 2017

View reviewed changes

stuartnelson3 merged commit 6088483 into prometheus:master Dec 22, 2017

carlpett mentioned this pull request Dec 28, 2017

Switch alertmanager to kingpin #974

Merged

sinkingpoint mentioned this pull request Jan 8, 2018

amtool no longer takes duration units greater than hours #1183

Closed

sinkingpoint mentioned this pull request Jan 10, 2018

Fix updating silence comments #1189

Merged

This was referenced Apr 16, 2018

amtool doesn't depend on a stable version of kingpin #1328

Closed

[amtool] use kingpin.v2 #1330

Merged

Conversation

carlpett commented Sep 4, 2017

Uh oh!

stuartnelson3 commented Sep 5, 2017

Uh oh!

Kellel commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kellel commented Sep 5, 2017

Uh oh!

Kellel commented Sep 5, 2017

Uh oh!

carlpett commented Sep 5, 2017

Uh oh!

carlpett commented Dec 17, 2017

Uh oh!

stuartnelson3 commented Dec 18, 2017

Uh oh!

carlpett commented Dec 19, 2017

Uh oh!

carlpett commented Dec 19, 2017

Uh oh!

stuartnelson3 commented Dec 20, 2017

Uh oh!

stuartnelson3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlpett commented Dec 20, 2017

Uh oh!

stuartnelson3 commented Dec 21, 2017

Uh oh!

carlpett commented Dec 21, 2017

Uh oh!

stuartnelson3 commented Dec 22, 2017

Uh oh!

stuartnelson3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kellel commented Sep 5, 2017 •

edited

Loading