feat(cli): dynamically generate query CLI commands #11725

aaronc · 2022-04-22T00:08:23Z

Description

This PR creates a new go.mod client/v2 which starts out with functionality to dynamically generate query CLI commands based on a protobuf Query service definitions.

The intention is to completely (or almost completely) replace manually generated CLI commands with this.

All request fields are turned into flags and positional parameters aren't used at all in this initial version. I actually think this is preferable and that positional parameters are confusing, but customization around this could be added in the future.

Handling of flag types can be customized based on the protobuf message type and cosmos_proto.scalar annotations. Out of the box handling is provided for:

all primitive protobuf types (int32, string, bool, etc.)
enums
arbitrary messages as JSON strings
repeated types (except repeated bytes)
proto Timestamp and Duration
cosmos.AddressString scalars
pagination PageRequest

In follow-up PRs, I plan to add:

custom Coin and Coins handling
support for JSON files for embedded messages
bech32 validation of address strings + resolution of key names in the keyring
support for generated tx commands
better integration with existing client.Context (until we can fully replace it)

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)
[ ]

ryanchristo

This is looking good. My main concern at the moment is the information that is lost or that would need to be added to the proto comments for the query messages. How do we auto-generate cli commands without losing the benefits of using cobra directly?

ryanchristo · 2022-04-25T03:59:16Z

client/v2/cli/query.go

+// CreateQueryMethodCommand creates a gRPC query command for the given service method.
+func (b *Builder) CreateQueryMethodCommand(descriptor protoreflect.MethodDescriptor) *cobra.Command {
+	serviceDescriptor := descriptor.Parent().(protoreflect.ServiceDescriptor)
+	docs := util.DescriptorDocs(descriptor)


I don't think the proto comment will serve as an adequate Long description. Many of the query commands we've implemented include more information than what is provided with the proto comment and the description is specific to the command. We also provide examples of how to use the command. If we were to use this as is, we would lose a lot of valuable information. How might custom Long descriptions be added?

ryanchristo · 2022-04-25T04:10:43Z

client/v2/cli/query.go

+	cmd := &cobra.Command{
+		Use:  protoNameToCliName(descriptor.Name()),
+		Long: docs,
+	}


What about Short, Example, and other cobra command properties? How might these be supported?

Ryan, would you be OK with the tradeoff implied here?

eg: we lose cobra configurability

I think we should find a solution to support these options but we can address in follow-ups. We recently started using cobra/doc in our application to generate documentation for each command and we are in the process of updating our commands to include such options as Example so that we can provide better documentation.

faddat · 2022-04-25T10:41:28Z

@aaronc thank you, I think this is fantastic

anilcse · 2022-04-25T12:42:58Z

Love this, let's automate more <3

aaronc · 2022-04-25T13:53:36Z

We can always add options to the proto files directly related to cli commands such as Usage and Short. Or that can be configured elsewhere. The benefit of storing it in the proto files is that it will allow a tool like lens to dynamically generate the CLI for any module even ones that have never been seen before.

aaronc · 2022-04-25T14:49:41Z

What about something like this:

package cosmos.bank.v1beta1;

service Query {
  // Balance queries the balance of a single coin for a single account.
  rpc Balance(QueryBalanceRequest) returns (QueryBalanceResponse) {
    option (cosmos.base.cli.v1.long) = "....";

  }

...
}

message QueryBalanceRequest {
  // address is the address to query balances for.
  string address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];

  // denom is the coin denom to query balances for.
  string denom = 2 [(cosmos.base.cli.v1.usage) = "...", (cosmos.base.cli.v1.shorthand) = "d"];
}

For commands themselves we could have the following annotations: long, short, aliases, example, maybe more if it's useful.
For fields that get turned into flags we can have name, shorthand, usage, and `default.

How does that sound?

Either way, is it okay if we deal with this configurability in follow-ups?

amaury1093 · 2022-04-25T15:10:26Z

I'm currently not convinced we need both lens and this CLI v2. For me this code should go in lens, and whether lens gets the file/service descriptors by loading from memory (like in this PR) or from reflection over network, should be a simple lens-side config.

Then, one day, the SDK simapp's CLI could do simapp lens ...

ryanchristo

Either way, is it okay if we deal with this configurability in follow-ups?

Yea, I think addressing this in follow-ups would be ok. This looks like the right direction and would be a really nice improvement. I just want to make sure we are providing good documentation both in the command interface itself and when auto-generating documentation using cobra/doc, both of which are dependent on these configuration options.

ryanchristo · 2022-04-25T15:11:00Z

client/v2/cli/query.go

+	cmd := &cobra.Command{
+		Use:  protoNameToCliName(descriptor.Name()),
+		Long: docs,
+	}


I think we should find a solution to support these options but we can address in follow-ups. We recently started using cobra/doc in our application to generate documentation for each command and we are in the process of updating our commands to include such options as Example so that we can provide better documentation.

aaronc · 2022-04-25T15:40:37Z

I'm currently not convinced we need both lens and this CLI v2. For me this code should go in lens, and whether lens gets the file/service descriptors by loading from memory (like in this PR) or from reflection over network, should be a simple lens-side config.

Then, one day, the SDK simapp's CLI could do simapp lens ...

I've heard talk of the lens code making it upstream into the SDK which I'd support. As far as I know there is no completely dynamic CLI generation in lens yet that fully uses regular CLI flags in a way that's similar to how we're currently using cobra. I've let the lens team know about this PR so hopefully we can align efforts.

In the short term, the goal is for this to provide immediate value for people writing regular modules and hand-written CLI commands. The idea is for this to be a drop-in replacement for the hand-written CLI that can be used ASAP and obviate the need to write any of that tedious and hard to test code.

aaronc · 2022-04-25T17:45:42Z

@ryanchristo starting adding proto options in #11763

amaury1093 · 2022-04-25T18:45:48Z

Re new proto options for CLI descriptions, I'm actually also not 100% in favor. I think it should be a separate config file. Let's maybe align on this tomorrow during our framework sync? Before Ryan puts too much work into it.

aaronc · 2022-04-25T19:05:40Z

Re new proto options for CLI descriptions, I'm actually also not 100% in favor. I think it should be a separate config file. Let's maybe align on this tomorrow during our framework sync? Before Ryan puts too much work into it.

Having options in the .proto files could make them more bloated. The advantage of the .proto files, however, is that they would get dynamically served up with gRPC reflection and make it easy to generate the client in a lens type context. We could also have a separate API to serve up these config files but that's another layer of complexity. Alternatively, dynamic clients like lens will just have less feature rich CLIs.

aaronc · 2022-04-26T20:07:52Z

In the Framework WG call today we agreed that this approach mostly makes sense but that we're not sure that .proto files are the right place for these options because of the risk of bloating those files. As a follow-up, we should probably look at a small PR showing annotations in .proto files and .yaml files side by side.

amaury1093

Code LGTM. i think this code is superior than lens, as in it's more chain-agnostic (whereas lens still hardcodes stuff). This should be the direction a cross-chain CLI should go towards.

My concern is more ecosystem-wise: what's the CLI story in cosmos? My ideal long-term vision is that there will be one CLI for the whole ecosystem. Would love to see efforts aligned between this and lens.

amaury1093 · 2022-04-27T14:16:05Z

As a follow-up, we should probably look at a small PR showing annotations in .proto files and .yaml files side by side.

A 3rd solution would be to add a new arg to AddQueryServiceCommands, which add those cobra long/short descriptions. We wouldn't need to embed extra files.

aaronc · 2022-04-27T14:20:14Z

As a follow-up, we should probably look at a small PR showing annotations in .proto files and .yaml files side by side.

A 3rd solution would be to add a new arg to AddQueryServiceCommands, which add those cobra long/short descriptions. We wouldn't need to embed extra files.

Yep, let's figure out customization in follow-ups. I think it depends on how much a dynamic client like lens would want to generate these fully-fleshed cobra commands on the fly

aaronc · 2022-04-27T16:16:43Z

What is going on with CI? This doesn't touch any existing code so why is test-race failing?

aaronc · 2022-04-27T22:25:33Z

I force merged this because unrelated CI was failing but can someone please look into what's wrong with CI @AmauryM @alexanderbez @robert-zaremba @marbar3778 ?

alexanderbez · 2022-04-27T22:27:36Z

Is this consistently failing? Or just a timeout? If it's flakey, let's add it to the tracking issue for flakey tests.

aaronc · 2022-04-27T22:31:20Z

Is this consistently failing? Or just a timeout? If it's flakey, let's add it to the tracking issue for flakey tests.

It seems like either it times out or some report isn't getting generating. Where is that tracking issue?

amaury1093 · 2022-04-28T09:37:03Z

#9010. This PR's failing test doesn't seem flaky though, but consistently failing.

aaronc added 4 commits April 21, 2022 18:31

WIP on auto-generating CLi

b954f4b

WIP

199a0f1

WIP

e1c39ba

WIP

b31fefd

github-actions bot added the C:CLI label Apr 22, 2022

aaronc added 6 commits April 22, 2022 11:56

add pagination.go

795ed9b

handle more flag types

b616755

WIP on refactoring

325004e

WIP

ca6bd8a

working tests

50d8fc5

add docs

8ea8876

aaronc marked this pull request as ready for review April 22, 2022 21:48

aaronc requested a review from alexanderbez as a code owner April 22, 2022 21:48

Merge branch 'main' into aaronc/cli-v2

c0b7d5d

aaronc requested review from ryanchristo and technicallyty April 22, 2022 21:48

aaronc added 4 commits April 22, 2022 17:52

echo all flags

7584582

Merge remote-tracking branch 'origin/aaronc/cli-v2' into aaronc/cli-v2

9ee2193

add repeated tests

d4bb040

remove comment

23c0333

aaronc requested a review from fdymylja April 22, 2022 22:36

fix compositeListValue issue

809848b

ryanchristo reviewed Apr 25, 2022

View reviewed changes

Merge branch 'main' into aaronc/cli-v2

0e3c714

blushi assigned amaury1093 and blushi Apr 25, 2022

ryanchristo approved these changes Apr 25, 2022

View reviewed changes

aaronc and others added 2 commits April 25, 2022 11:41

Merge branch 'main' into aaronc/cli-v2

076c718

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/cli-v2

1d42050

Merge branch 'main' into aaronc/cli-v2

4d3c23d

aaronc mentioned this pull request Apr 26, 2022

EPIC: AutoCLI & hubl #11775

Closed

33 tasks

aaronc added 2 commits April 26, 2022 16:07

Merge branch 'main' into aaronc/cli-v2

9a524cb

Merge branch 'main' into aaronc/cli-v2

3d9de76

aaronc requested a review from amaury1093 April 27, 2022 14:05

amaury1093 approved these changes Apr 27, 2022

View reviewed changes

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 27, 2022

Merge branch 'main' into aaronc/cli-v2

43f01bb

aaronc and others added 3 commits April 27, 2022 13:44

Merge branch 'main' into aaronc/cli-v2

28ff519

Merge branch 'main' into aaronc/cli-v2

e807d28

Merge branch 'main' into aaronc/cli-v2

6f5c21b

aaronc merged commit 1c8a2d9 into main Apr 27, 2022

aaronc deleted the aaronc/cli-v2 branch April 27, 2022 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): dynamically generate query CLI commands #11725

feat(cli): dynamically generate query CLI commands #11725

aaronc commented Apr 22, 2022 •

edited

Loading

ryanchristo left a comment

ryanchristo Apr 25, 2022

ryanchristo Apr 25, 2022 •

edited

Loading

faddat Apr 25, 2022

ryanchristo Apr 25, 2022

faddat commented Apr 25, 2022

anilcse commented Apr 25, 2022

aaronc commented Apr 25, 2022

aaronc commented Apr 25, 2022

amaury1093 commented Apr 25, 2022

ryanchristo left a comment

ryanchristo Apr 25, 2022

aaronc commented Apr 25, 2022 •

edited

Loading

aaronc commented Apr 25, 2022

amaury1093 commented Apr 25, 2022

aaronc commented Apr 25, 2022

aaronc commented Apr 26, 2022

amaury1093 left a comment

amaury1093 commented Apr 27, 2022

aaronc commented Apr 27, 2022

aaronc commented Apr 27, 2022

aaronc commented Apr 27, 2022

alexanderbez commented Apr 27, 2022

aaronc commented Apr 27, 2022

amaury1093 commented Apr 28, 2022

feat(cli): dynamically generate query CLI commands #11725

feat(cli): dynamically generate query CLI commands #11725

Conversation

aaronc commented Apr 22, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo Apr 25, 2022

Choose a reason for hiding this comment

ryanchristo Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

faddat Apr 25, 2022

Choose a reason for hiding this comment

ryanchristo Apr 25, 2022

Choose a reason for hiding this comment

faddat commented Apr 25, 2022

anilcse commented Apr 25, 2022

aaronc commented Apr 25, 2022

aaronc commented Apr 25, 2022

amaury1093 commented Apr 25, 2022

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo Apr 25, 2022

Choose a reason for hiding this comment

aaronc commented Apr 25, 2022 • edited Loading

aaronc commented Apr 25, 2022

amaury1093 commented Apr 25, 2022

aaronc commented Apr 25, 2022

aaronc commented Apr 26, 2022

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 commented Apr 27, 2022

aaronc commented Apr 27, 2022

aaronc commented Apr 27, 2022

aaronc commented Apr 27, 2022

alexanderbez commented Apr 27, 2022

aaronc commented Apr 27, 2022

amaury1093 commented Apr 28, 2022

aaronc commented Apr 22, 2022 •

edited

Loading

ryanchristo Apr 25, 2022 •

edited

Loading

aaronc commented Apr 25, 2022 •

edited

Loading