config: refactor commands to print help for flags (#3536) #3538

magiconair · 2017-10-05T13:26:15Z

This patch refactors the commands that use the mitchellh/cli library to
populate the command line flag set in both the Run() and the Help()
method. Earlier versions of the mitchellh/cli library relied on the
Run() method to populuate the flagset for generating the usage screen.
This has changed in later versions and was previously solved with a
small monkey patch to the library to restore the old behavior.

However, this makes upgrading the library difficult since the patch has
to be restored every time.

This patch addresses this by moving the command line flags into an
initFlags() method where appropriate and also moving all variables for
the flags from the Run() method into the command itself.

Fixes #3536

preetapan · 2017-10-17T12:48:30Z

command/agent/agent_test.go

@@ -18,7 +18,6 @@ import (



Was running these in parallel breaking something? The tests were passing on this branch for me when I last touched it.

no, but it wasn't helping either since go test already does package level parallelization. My CPU burned through all the tests in the same time whether the tests were running in parallel or not.

Tried it again. Test run time goes from 55sec to 51sec when running tests in parallel. Maybe I should leave this in.

magiconair · 2017-10-17T13:51:42Z


Seems to be fixed.

slackpad

A few minor things to clean up but the approach LGTM. Once this is merged can you please take it over to the consul-enterprise fork? There will be a minor merge conflict in commands.go and there are a few commands over there that will need to get migrated.

slackpad · 2017-10-17T20:15:40Z

command/forceleave/forceleave.go

+	c.http = &flags.HTTPFlags{}
+	flags.Merge(c.flags, c.http.ClientFlags())
+	flags.Merge(c.flags, c.http.ServerFlags())
+	c.help = flags.Usage(help, c.flags, c.http.ClientFlags(), c.http.ServerFlags())


Why do we have to repeat c.http.ClientFlags(), c.http.ServerFlags() in Usage() if they are already merged into c.flags?

c.flags contains the total list of command line flags the command accepts. For the usage we want to group the flags by HTTP API Options and Command Options. Unless we hard-code the names in the flags.Usage method we cannot tell which flag is an HTTP API flag or a Command flag. The BaseCommand did this under the hood by using some bit flags to determine whether a command had HTTP API flags in addition to the command flags. I went through a number of different approaches. I can see whether I can come up with a better approach since this is a cause of errors.

$ bin/consul event -h Usage: consul event [options] [payload] Dispatches a custom user event across a datacenter. An event must provide a name, but a payload is optional. Events support filtering using regular expressions on node name, service, and tag definitions. HTTP API Options -ca-file=<value> Path to a CA file to use for TLS when communicating with Consul. This can also be specified via the CONSUL_CACERT environment variable. -ca-path=<value> Path to a directory of CA certificates to use for TLS when communicating with Consul. This can also be specified via the CONSUL_CAPATH environment variable. -client-cert=<value> Path to a client cert file to use for TLS when 'verify_incoming' is enabled. This can also be specified via the CONSUL_CLIENT_CERT environment variable. -client-key=<value> Path to a client key file to use for TLS when 'verify_incoming' is enabled. This can also be specified via the CONSUL_CLIENT_KEY environment variable. -http-addr=<address> The `address` and port of the Consul HTTP agent. The value can be an IP address or DNS address, but it must also include the port. This can also be specified via the CONSUL_HTTP_ADDR environment variable. The default value is http://127.0.0.1:8500. The scheme can also be set to HTTPS by setting the environment variable CONSUL_HTTP_SSL=true. -tls-server-name=<value> The server name to use as the SNI host when connecting via TLS. This can also be specified via the CONSUL_TLS_SERVER_NAME environment variable. -token=<value> ACL token to use in the request. This can also be specified via the CONSUL_HTTP_TOKEN environment variable. If unspecified, the query will default to the token of the Consul agent at the HTTP address. Command Options -name=<string> Name of the event. -node=<string> Regular expression to filter on node names. -service=<string> Regular expression to filter on service instances. -tag=<string> Regular expression to filter on service tags. Must be used with -service.

I can simplify that.

I do like the explicit merge approach vs. the bitfields. I'm wondering if we can search for the ClientFlags and ServerFlags when we render the usage and group them, and filter them out otherwise, since we know what they are.

You're right. With the current code this is no longer necessary. I've refactored the commands so that they extract the HTTP API flags automatically.

slackpad · 2017-10-17T20:17:31Z

command/forceleave/forceleave.go

+	c.http = &flags.HTTPFlags{}
+	flags.Merge(c.flags, c.http.ClientFlags())
+	flags.Merge(c.flags, c.http.ServerFlags())
+	c.help = flags.Usage(help, c.flags, c.http.ClientFlags(), c.http.ServerFlags())


This is an agent-level command so should just have ClientFlags but not ServerFlags.

slackpad · 2017-10-17T20:36:17Z

command/keyring/keyring.go

-	if err := c.BaseCommand.Parse(args); err != nil {
+	c.http = &flags.HTTPFlags{}
+	flags.Merge(c.flags, c.http.ClientFlags())
+	flags.Merge(c.flags, c.http.ServerFlags())


No server, just client for this one.

slackpad · 2017-10-17T20:39:03Z

command/leave/leave.go

+	c.flags = flag.NewFlagSet("", flag.ContinueOnError)
+	c.http = &flags.HTTPFlags{}
+	flags.Merge(c.flags, c.http.ClientFlags())
+	flags.Merge(c.flags, c.http.ServerFlags())


No server flags on this one.

slackpad · 2017-10-17T20:43:06Z

command/reload/reload.go

+	c.http = &flags.HTTPFlags{}
+	flags.Merge(c.flags, c.http.ClientFlags())
+	flags.Merge(c.flags, c.http.ServerFlags())
+	c.help = flags.Usage(help, c.flags, c.http.ClientFlags(), c.http.ServerFlags())


No server on this one.

slackpad · 2017-10-17T20:46:10Z

vendor/github.com/mitchellh/cli/cli.go

@@ -578,6 +578,12 @@ func (c *CLI) processArgs() {
 			break
 		}

+		// Check for help flags.
+		if arg == "-h" || arg == "-help" || arg == "--help" {


Should this be in this PR or is it a no-op with the de-monkey-patch change that just got merged?

This is the reversal of the monkey patch which was the motivation for this PR in the first place and it should be in this PR. The other de-monkey-patch was for mitchellh/mapstructure not mitchellh/cli.

Got it - ok.

magiconair · 2017-10-17T21:17:26Z

I'll do the enterprise code as well.

This patch refactors the commands that use the mitchellh/cli library to populate the command line flag set in both the Run() and the Help() method. Earlier versions of the mitchellh/cli library relied on the Run() method to populuate the flagset for generating the usage screen. This has changed in later versions and was previously solved with a small monkey patch to the library to restore the old behavior. However, this makes upgrading the library difficult since the patch has to be restored every time. This patch addresses this by moving the command line flags into an initFlags() method where appropriate and also moving all variables for the flags from the Run() method into the command itself. Fixes #3536

Package level parallelization is sufficient.

* move Help and Synopsis to bottom * make help and synopsis constants * make sure help output is formatted

slackpad

LGTM

magiconair · 2017-10-17T22:08:35Z

Here we go!

magiconair requested review from slackpad, preetapan and kyhavlov October 5, 2017 13:26

magiconair mentioned this pull request Oct 5, 2017

Consul CLI commands don't display detailed help #3536

Closed

magiconair force-pushed the issue-3536-fix-cli-help branch from 6ed138c to ab6b970 Compare October 6, 2017 10:49

magiconair self-assigned this Oct 6, 2017

magiconair force-pushed the issue-3536-fix-cli-help branch 8 times, most recently from 912a86a to 98c2faf Compare October 11, 2017 14:49

magiconair removed request for preetapan, slackpad and kyhavlov October 11, 2017 14:57

preetapan force-pushed the issue-3536-fix-cli-help branch from 98c2faf to 0083f1a Compare October 11, 2017 17:45

magiconair force-pushed the issue-3536-fix-cli-help branch from 9541818 to f028b92 Compare October 17, 2017 11:03

preetapan reviewed Oct 17, 2017

View reviewed changes

magiconair force-pushed the issue-3536-fix-cli-help branch from e4721d1 to bafa5d4 Compare October 17, 2017 14:10

slackpad reviewed Oct 17, 2017

View reviewed changes

magiconair added 6 commits October 18, 2017 00:06

revert monkey patch for mitchellh/cli

c68ba31

commands: move flag handling into flags pkg

c70fcfd

commands: move validate command to separate pkg

7e1cd33

commands: move join command to separate pkg

1499c3d

commands: move event command to separate pkg

bc21d26

magiconair added 24 commits October 18, 2017 00:06

commands: move snapshot restore command to separate pkg

0482b3d

commands: move snapshot save command to separate pkg

3906719

add token and addr to http flags

b0ebfc5

commands: move watch command to separate pkg

9491958

commands: move agent command to separate pkg

b1164f9

commands: drop base command and utils

e30421e

commands: simplify commands.go

ba54e21

commands: move snapshot subcommands to subdirs

d03a5f9

commands: move catalog subcommands to subdirs

e9c9bdb

commands: move kv subcommands to subdirs

9f75ed0

commands: move operator subcommands to subdirs

50bcb95

commands: simplify import names

22b6ca6

commands: cleanup catalog list nodes tests

7759849

commands: cleanup catalog list services tests

df24c5b

commands: cleanup test names

1f95c31

commands: add missing noTabs test

c4f6109

commands: do not run cmd tests in parallel

d782d14

Package level parallelization is sufficient.

commands: cleanup help and synopsis.

26ea2a8

* move Help and Synopsis to bottom * make help and synopsis constants * make sure help output is formatted

commands: run all tests in parallel (again)

318cbfc

commands: drop http server flags from force-leave command

71f106f

commands: drop http server flags from keyring command

7e2968a

commands: drop http server flags from leave command

108a823

commands: drop http server flags from reload command

7e44478

commands: get HTTP API flags for usage automatically

0543f65

magiconair force-pushed the issue-3536-fix-cli-help branch from 55fc684 to 0543f65 Compare October 17, 2017 22:07

slackpad approved these changes Oct 17, 2017

View reviewed changes

magiconair merged commit b3292d1 into master Oct 17, 2017

magiconair deleted the issue-3536-fix-cli-help branch October 17, 2017 22:08

magiconair added this to the 1.0.1 milestone Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: refactor commands to print help for flags (#3536) #3538

config: refactor commands to print help for flags (#3536) #3538

magiconair commented Oct 5, 2017

preetapan Oct 17, 2017

magiconair Oct 17, 2017

magiconair Oct 17, 2017

magiconair commented Oct 17, 2017 •

edited

Loading

slackpad left a comment

slackpad Oct 17, 2017

magiconair Oct 17, 2017

magiconair Oct 17, 2017

slackpad Oct 17, 2017

magiconair Oct 17, 2017

slackpad Oct 17, 2017

magiconair Oct 17, 2017

slackpad Oct 17, 2017

magiconair Oct 17, 2017

slackpad Oct 17, 2017

slackpad Oct 17, 2017

magiconair Oct 17, 2017

slackpad Oct 17, 2017

magiconair Oct 17, 2017 •

edited

Loading

slackpad Oct 17, 2017

magiconair commented Oct 17, 2017

slackpad left a comment

magiconair commented Oct 17, 2017

config: refactor commands to print help for flags (#3536) #3538

config: refactor commands to print help for flags (#3536) #3538

Conversation

magiconair commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented Oct 17, 2017 • edited Loading

slackpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented Oct 17, 2017

slackpad left a comment

Choose a reason for hiding this comment

magiconair commented Oct 17, 2017

magiconair commented Oct 17, 2017 •

edited

Loading

magiconair Oct 17, 2017 •

edited

Loading