Add catalog CLI functions #3204

sethvargo · 2017-06-29T00:50:24Z

I am restricting the first pass to "readonly" (no register or deregister functionality).

/cc @hashicorp/consul

sethvargo · 2017-06-29T00:51:22Z

command/catalog_command.go

+
+  List all datacenters:
+
+      $ consul catalog datacenters


❓ should these all be "actions" instead? Like list-datacenters and list-nodes? In the consul kv CLI, everything is an action (verb), but list-datacenters seems blah to me.

I think just datacenters is fine since the only datacenter-related thing you can do is list them right now, if we added other actions later it could become consul catalog datacenters list.

Yeah I kind of like the verbs (like https://www.consul.io/docs/commands/operator/area.html), we could start out that way so we get help with the top-level command showing help and we've got a spot for more things later like deregister:

consul catalog datacenters list consul catalog nodes list consul catalog services list

If we start out with consul catalog datacenters running list, there's no where to park the help for the datacenters subcommands later, for example.

Yea, that was my fear. I just also fear that we are going too deep (queue Inception gif). I find CLIs challenging to reason about beyond the third level, which is why I like the kv CLI

consul kv [action]

That's why I suggested

consul catalog list-datacenters

since then list is a verb. Otherwise, we have inconsistency where some CLI commands are "namespace verb" and others are "namespace noun verb".

Hmm - I can live with that - I do like verbs and flattening the verbs into one level of consul catalog <verb> seems like a good compromise.

sethvargo · 2017-06-29T00:52:03Z

command/catalog_datacenters.go

+	"github.com/mitchellh/cli"
+)
+
+var _ cli.Command = (*CatalogDatacentersCommand)(nil)


@kyhavlov I found this on the Internets as a better way to test is something implements an interface. We used to do this in test, but this will fail at build time if the interfaces don't match.

Good find, that's a lot better than having to run a test

sethvargo · 2017-06-29T00:54:57Z

command/catalog_nodes.go

+	return strings.TrimSpace(helpText)
+}
+
+func (c *CatalogNodesCommand) Run(args []string) int {


This is still a WIP, and I wanted to solicit feedback on the API/UX here.

I can see a few possible ways this could go:

consul catalog nodes returns a list of node ids (no other data), and then -detailed returns a more tabular like (like consul kv get -detailed)

should it be node ids or node names?

should it be node names falling back to node ids?

consul catalog nodes returns everything in tabular format except tagged addrs, meta, and create/modify indexes

Thoughts? I think I like the first one, since it allows for better composition into other resources.

Having it show tabular format similar to members (option 2) could still feed into cut or some other utility, but I don't really have a preference here, as long as there's a -detailed option.

I was kind of expecting consul members-like table output for this, too, since there's a lot of info vs consul kv get -detailed. Maybe something like:

consul catalog node list maybe a table of: node name, node id, address

consul catalog node info -node=<name or id> does the kv -detailed kind of thing

Still kind of thinking about this one...

sethvargo · 2017-06-29T00:55:12Z

command/catalog_nodes.go

+func prettyNode(w io.Writer, node *api.Node) error {
+	tw := tabwriter.NewWriter(w, 0, 2, 6, ' ', 0)
+	fmt.Fprintf(tw, "ID\t%s\n", node.ID)
+	if node.Node != "" {


Can this ever be empty?

If you try to start a node with a blank node name, it'll default to the hostname, so I don't think this should ever be empty.

slackpad

Looking good - added some thoughts!

slackpad · 2017-06-29T23:07:00Z

command/catalog_command.go

+
+  List all datacenters:
+
+      $ consul catalog datacenters


Yeah I kind of like the verbs (like https://www.consul.io/docs/commands/operator/area.html), we could start out that way so we get help with the top-level command showing help and we've got a spot for more things later like deregister:

consul catalog datacenters list consul catalog nodes list consul catalog services list

If we start out with consul catalog datacenters running list, there's no where to park the help for the datacenters subcommands later, for example.

slackpad · 2017-06-29T23:07:42Z

command/catalog_nodes.go

+
+func (c *CatalogNodesCommand) Help() string {
+	helpText := `
+Usage: consul catalog datacenters [options]


slackpad · 2017-06-29T23:18:01Z

command/catalog_nodes.go

+	return strings.TrimSpace(helpText)
+}
+
+func (c *CatalogNodesCommand) Run(args []string) int {


I was kind of expecting consul members-like table output for this, too, since there's a lot of info vs consul kv get -detailed. Maybe something like:

consul catalog node list maybe a table of: node name, node id, address

consul catalog node info -node=<name or id> does the kv -detailed kind of thing

Still kind of thinking about this one...

sethvargo · 2017-07-10T14:33:11Z

$ consul catalog list-datacenters
dc1
dc2

$ consul catalog list-nodes
Node       ID        Address    DC
bacon-mac  d4e68c58  127.0.0.1  dc1

$ consul catalog list-nodes -service=consul
Node       ID        Address    DC
bacon-mac  d4e68c58  127.0.0.1  dc1

$ consul catalog list-services
consul
redis

$ consul catalog list-services -tags
consul
redis       primary,v1

@slackpad @kyhavlov @magiconair @preetapan I think this is ready for real review now! 😄

sethvargo · 2017-07-10T15:09:11Z

Also, the more I'm typing and working with these, I think list is the wrong prefix 😦 . It's really painful to type each time, especially given there's no show- or update-, etc.

$ consul catalog datacenters

$ consul catalog nodes

$ consul catalog servies

Feels a lot better than

$ consul catalog list-datacenters

$ consul catalog list-nodes

$ consul catalog list-services

I've been using this pretty regularly for about a week, and I don't think list- is provide value or future-proofing ourselves. Thoughts?

slackpad · 2017-07-10T15:15:01Z

@sethvargo that's fair - i'd be ok to drop the "list-" prefix as it does look ugly. We could put verbs for future actions and mix them, which seems workable:

$ consul catalog deregister -node=foo -service=bar

sethvargo · 2017-07-10T16:05:48Z

Okay - updated

This was just taking up bytes given the newer flag format

This isn't exactly "pretty". It's much easier to use the same data structure, so we convert the unmatched data type to the proper one.

It's possible to generate an empty node ID using the catalog/register endpoint.

This actually calls up to the servers, so want to not include "agent" here.

sethvargo commented Jun 29, 2017

View reviewed changes

slackpad reviewed Jun 29, 2017

View reviewed changes

sethvargo force-pushed the sethvargo/catalog_cli branch from 0177c9d to 5c38350 Compare July 10, 2017 14:05

sethvargo requested review from magiconair and preetapan July 10, 2017 14:33

sethvargo added 18 commits July 14, 2017 12:12

Add flag for map[string]string

0da723a

Remove old api helper

cbc07a0

This was just taking up bytes given the newer flag format

Add scaffold for catalog datacenters

6023bdf

WIP: Add catalog nodes

a665743

Use list-* prefixes, table-like output

8dbdc25

Add catalog list-services

80a962c

Order catalog services list since map ordering is unpredictable

1bfc756

Add filtering services by node

0d8a2aa

This isn't exactly "pretty". It's much easier to use the same data structure, so we convert the unmatched data type to the proper one.

Allow filtering nodes by service

4e636d5

Lowercase cosmetic

c3569db

Handle empty node IDs

21c49f2

It's possible to generate an empty node ID using the catalog/register endpoint.

Use logical service names instead of real services

04ebee6

Minor cosmetic help output

d2a96b8

Give all flags

b34ccba

Add docs

f37ad75

Remove list- prefix

1c6966e

Fix typo

b1df10b

Remove unused footer links

f827602

Fix broken link during mass cpr

ef4d322

sethvargo force-pushed the sethvargo/catalog_cli branch from 500d50c to ef4d322 Compare July 14, 2017 16:16

slackpad added 3 commits July 14, 2017 12:33

Adds a missing colon.

99cf07d

Tweaks synopsis.

f7a7050

This actually calls up to the servers, so want to not include "agent" here.

Tweaks a comment.

dac8fd9

slackpad merged commit afd83a9 into master Jul 14, 2017

slackpad deleted the sethvargo/catalog_cli branch July 14, 2017 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add catalog CLI functions #3204

Add catalog CLI functions #3204

sethvargo commented Jun 29, 2017 •

edited

Loading

sethvargo Jun 29, 2017

kyhavlov Jun 29, 2017

slackpad Jun 29, 2017

sethvargo Jun 29, 2017

slackpad Jun 29, 2017

sethvargo Jun 29, 2017

kyhavlov Jun 29, 2017

sethvargo Jun 29, 2017

kyhavlov Jun 29, 2017

slackpad Jun 29, 2017

sethvargo Jun 29, 2017 •

edited

Loading

kyhavlov Jun 29, 2017

slackpad left a comment

slackpad Jun 29, 2017

slackpad Jun 29, 2017

slackpad Jun 29, 2017

sethvargo commented Jul 10, 2017 •

edited

Loading

sethvargo commented Jul 10, 2017

slackpad commented Jul 10, 2017

sethvargo commented Jul 10, 2017

Add catalog CLI functions #3204

Add catalog CLI functions #3204

Conversation

sethvargo commented Jun 29, 2017 • edited Loading

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

sethvargo Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

sethvargo commented Jul 10, 2017 • edited Loading

sethvargo commented Jul 10, 2017

slackpad commented Jul 10, 2017

sethvargo commented Jul 10, 2017

sethvargo commented Jun 29, 2017 •

edited

Loading

sethvargo Jun 29, 2017 •

edited

Loading

sethvargo commented Jul 10, 2017 •

edited

Loading