[amtool] use kingpin.v2 by simonpasquier · Pull Request #1330 · prometheus/alertmanager

simonpasquier · 2018-04-17T14:05:42Z

This is an attempt to fix #1328. To workaround the lack of configuration file support in kingpin v2, it injects environment variables after reading the flags from the YAML files. This isn't pretty but at least seems to work (this would need more testing obviously).

As noted in #976, there's no way to have longer help descriptions when using the --help-long flag. I've tried several approaches but failed. I don't think it is even possible. So switching to v2 would mean to get rid of either the short helps or the long helps.

Waiting for @stuartnelson3 & @thetincho's comments...

brian-brazil · 2018-04-17T14:51:41Z

cli/config_resolver.go

+)

-	resolver := configResolver{}
+// loadConfig loads flags from YAML files and injects them as environment variables.


Could any of these be secrets? The environment is not a safe place for secrets.

I see your concern. Right now amtool doesn't expose any secret flags or arguments. Which isn't saying it won't.

An idea would be to store this in a public map at init time and use it to populate defaults for flags.. but maybe it is too convoluted

The alertmanager.url can potentially contain basic auth credentials.

Any ideas on how else to implement this? @thetincho's map idea might work. Parse the file and populate a struct/map, and set those as the default values for their appropriate flags?

I've just tried and it doesn't work because of alertmanager.url (at least) which is a required flag . As stated in the kingpin documentation: You can not provide a Default() value to a Required() flag.

I've tried the suggestion from alecthomas/kingpin#208 (comment) but it doesn't work either because some flags are only valid for some commands and you can't have a flag present both in the configuration file and in the program's arguments (unless the flag can be repeated).

Could we move it from being a Required flag, provide the value in the config file, and validate after the fact ourselves?

EDIT:
The flag will still attempt to create a URL var, so maybe it'll be as simple as doing a nil check and erroring

@stuartnelson3 good idea, it seems to work...

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

NightTsarina · 2018-04-17T19:18:40Z

Thanks for this!

I have no comments on the PR, except my small suggestion.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

stuartnelson3 · 2018-04-20T13:57:27Z

This has the fantastic side-effect of allowing check-configto no longer require --alertmanager.url

#1199

stuartnelson3

a question and a small request, but looks awesome!

stuartnelson3 · 2018-04-20T14:01:50Z

cli/root.go

+			continue
+		}
+		name := f.Model().Name
+		if name == "help" || name == "help-long" || name == "help(man" {


what's help(man?

stuartnelson3 · 2018-04-20T14:03:23Z

cli/config/config.go

+			return nil, err
+		}
+		for k, v := range m {
+			if new, ok := legacyFlags[k]; ok {


it appears to be compiling here, but new is a keyword in golang and should be renamed

Thanks for the catch-up!

simonpasquier · 2018-04-20T14:30:14Z

Thanks @stuartnelson3, from my testing, the precedence between command line flags and configuration files works as expected but I plan to increase the coverage in the unit tests.

There's still a question around short vs. long help messages though.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

stuartnelson3 · 2018-04-23T09:13:39Z

As noted in #976, there's no way to have longer help descriptions when using the --help-long flag. I've tried several approaches but failed. I don't think it is even possible. So switching to v2 would mean to get rid of either the short helps or the long helps.

I would prefer to keep the longer help message. The documentation isn't too long, and it covers the use cases. We would have to figure out where to add the long help information on prometheus.io if we were to get rid of it.

simonpasquier · 2018-04-23T10:08:01Z

I would prefer to keep the longer help message.

This is my feeling too: having it buried under --help-long isn't very user-friendly. I'll update the PR in this direction then.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

stuartnelson3

one final nit!

stuartnelson3 · 2018-04-23T17:33:26Z

cli/root.go

 	var (
-		longHelpText = map[string]string{}
-		app          = kingpin.New("amtool", "Alertmanager CLI").DefaultEnvars()
+		app = kingpin.New("amtool", helpRoot).DefaultEnvars()


I think DefaultEnvars() can be removed, right?

It could be deleted although the current code already has DefaultEnvars() which is why I didn't change it.

k. I doubt anyone is relying on it, but I suppose if someone is we can leave it for now.

* Use default values to store values from config * fix typo and reserved keywork * move to long help texts * add one more unit test for resolver * update comments Signed-off-by: Simon Pasquier <spasquie@redhat.com>

brian-brazil reviewed Apr 17, 2018

View reviewed changes

cli: use kingpin.v2

bd0f8de

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the kingpin-v2 branch from 055562e to bd0f8de Compare April 17, 2018 15:25

Use default values to store values from config

b0f6fe4

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

stuartnelson3 suggested changes Apr 20, 2018

View reviewed changes

stuartnelson3 changed the title ~~[WIP] cli: use kingpin.v2~~ [amtool] use kingpin.v2 Apr 20, 2018

cli: fix typo and reserved keywork

1cee75f

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the kingpin-v2 branch from ec630f5 to 1cee75f Compare April 20, 2018 14:33

simonpasquier added 3 commits April 23, 2018 16:57

cli: move to long help texts

846d324

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

cli: add one more unit test for resolver

1e20010

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

cli: update comments

ce76bc3

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

stuartnelson3 mentioned this pull request Apr 23, 2018

Release Alertmanager v0.15.0 #1340

Closed

stuartnelson3 suggested changes Apr 23, 2018

View reviewed changes

stuartnelson3 approved these changes Apr 24, 2018

View reviewed changes

stuartnelson3 merged commit dc5fc02 into prometheus:master Apr 24, 2018

simonpasquier deleted the kingpin-v2 branch April 24, 2018 08:32

stuartnelson3 mentioned this pull request May 3, 2018

amtool requires --alertmanager.url for check-config in 0.13.0 #1199

Closed

Conversation

simonpasquier commented Apr 17, 2018

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!

stuartnelson3 Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NightTsarina commented Apr 17, 2018

Uh oh!

stuartnelson3 commented Apr 20, 2018

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!

simonpasquier commented Apr 20, 2018

Uh oh!

stuartnelson3 commented Apr 23, 2018

Uh oh!

simonpasquier commented Apr 23, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stuartnelson3 Apr 19, 2018 •

edited

Loading