Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --json flag to force JSON output in relayer CLI #500

Closed
5 tasks done
romac opened this issue Jan 5, 2021 · 10 comments · Fixed by #518
Closed
5 tasks done

Add --json flag to force JSON output in relayer CLI #500

romac opened this issue Jan 5, 2021 · 10 comments · Fixed by #518
Assignees
Labels
I: CLI Internal: related to the relayer's CLI
Milestone

Comments

@romac
Copy link
Member

romac commented Jan 5, 2021

Crate

relayer-cli

Summary

Add a --json flag to force JSON output in all the CLI commands.

Problem Definition

Why do we need this?

  • @andynog expressed the need for JSON output as part of his work on the CI
  • other users may want to use the CLI programmatically without relying on using regular expressions to parse the output of the commands

Proposal

  • Add a --json flag to all commands, which enables JSON output.
  • When this flag is set, a command should not use the status_ok!, status_info! and status_err! macros but instead emit a single-line JSON-encoded message.
  • The message must include a status field which must be set to one of "success", "info", or "error".
  • The message should include a msg field with a human-readable description of the result of the command.
  • The message may include other fields.

Example:

{"status": "success", "msg": "client created", "client_id": "tendermint-07"}
{"status": "info", "msg": "no action taken"}
{"status": "error", "msg": "client not found", "client_id": "tendermint-07"}

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@romac romac added the I: CLI Internal: related to the relayer's CLI label Jan 5, 2021
@romac romac added this to the v0.0.7 milestone Jan 5, 2021
@adizere adizere mentioned this issue Jan 5, 2021
8 tasks
@adizere
Copy link
Member

adizere commented Jan 5, 2021

Maybe also take into consideration the following problem highlighted elsewhere (in #469):

Sort-out error messages and make them consistent. For example, updating a non-existing client results in a panic, but querying for a non-existing client returns an error. The commands are:

rrly -c loop_config.toml tx raw update-client ibc-0 ibc-1 07-tendermint-X
rrly -c loop_config.toml query client state ibc-0 07-tendermint-X

@romac
Copy link
Member Author

romac commented Jan 5, 2021

Agreed!

On top of that, we should also return a proper UNIX error code on exit, via abscissa_core::application::fatal_error. See the light rm command for an example of how to do this.

@adizere
Copy link
Member

adizere commented Jan 8, 2021

I'm thinking we should make --json always on and simply stop using status_ok!, status_info! and status_err! macros altogether.

@ancazamfir
Copy link
Collaborator

I like the way the tx CLIs return the result of the transaction, decoded with the interesting information. It would be nice to include that. For example channel openInit is confirmed with the OpenInitChannel event included in the result. Maybe we can have something like:

{"status": "success",  "msg": {"OpenInitChannel": {"height": "1", "port_id": "transfer", "channel_id": "channel-3", "connection_id": "connection-0", "counterparty_port_id": "transfer", "counterparty_channel_id": null}}}

Or result instead of msg.
We also need to add similar format for queries (see @adizere 's #506) so we need a proposal to unify the Tx and query CLIs.

@romac
Copy link
Member Author

romac commented Jan 8, 2021

Sounds good.

So, should we say that:

  • the relayer must only output JSON (specifically, JSON Lines) on stdout
  • the relayer may output arbitrary text (including ANSI escape codes) on stderr

This would allow us to pipe the relayer stdout to jq while also retaining debug and tracing information on stderr.

What do you think?

@romac
Copy link
Member Author

romac commented Jan 8, 2021

If we stick with the tracing crate, we may be able to support both human-readable output and JSON output by configuring the tracing subscriber at runtime, eg. based on a command-line flag, or an environment variable.

What remains to be seen is if that's doable with Abscissa,

@adizere
Copy link
Member

adizere commented Jan 11, 2021

I like the idea of configuring the output format at runtime. After a bit of digging, Abscissa has support for verbose and help global flags, but does not support, out-of-the-box, something like a global json command-line flag. So configuring JSON globally for the whole application seems easier based on environment variables.

But to focus on @andynog's work on CI, I'm curious what's important there..

  • Is JSON output necessary for queries or txs or both?
  • Is non-JSON output ever useful in the CI?
  • Is anything beside the "status" field in the JSON output parsed by the CI?

@ancazamfir
Copy link
Collaborator

Is JSON output necessary for queries or txs or both?

Both

Is non-JSON output ever useful in the CI?

Probably not

Is anything beside the "status" field in the JSON output parsed by the CI?

Yes, for example IDs should be extracted from the init/ create Tx results and used in subsequent CLIs. Verification should also issue query commands and check fields from the output.

@ancazamfir
Copy link
Collaborator

Since Andy is working on CI already, can we just default everything to json for now and we look at a nicer output for us/ human users later? The json output is actually not too bad.

@andynog
Copy link
Contributor

andynog commented Jan 11, 2021

I agree that Json output for tx and query should be the default. Eventually we might have a better configurable logging mechanism (support multiple log outputs and formats).

But for now, automating things will be easier I believe using Json and is the way to go. It's easier to programmatically extract values then trying to parse text which is difficult and error prone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: CLI Internal: related to the relayer's CLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants