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

feat(cli): status cmd cli support output text #17184

Merged
merged 8 commits into from
Aug 1, 2023

Conversation

zakir-code
Copy link
Contributor

  • Deprecated clientCtx.LegacyAmino
  • Add json tag for consistency with /status api

@zakir-code zakir-code requested a review from a team as a code owner July 29, 2023 09:25
@github-actions github-actions bot added the C:CLI label Jul 29, 2023
@github-prbot github-prbot requested review from a team, kocubinski and testinginprod and removed request for a team July 29, 2023 09:25
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an ok UX improvement, however this is a CLI breaking change (keys are changing and output is now by default YAML).
I think we should leave the default format output json by default for limiting the breaking changes.
We need a changelog for that under unreleased CLI Breaking Changes for explaining the keys naming change.

client/rpc/status.go Outdated Show resolved Hide resolved
@zakir-code
Copy link
Contributor Author

zakir-code commented Jul 29, 2023

The modification here will affect the output type of validator_info.address, which is base64 before modification and hex after modification

@zakir-code
Copy link
Contributor Author

Here is another address type output

Address: sdk.ConsAddress(validator.Address),

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Can we get a CLI Breaking Change changelog?

For other reviewers:

current:

{"NodeInfo":{"protocol_version":{"p2p":"8","block":"11","app":"0"},"id":"af727ae22dedb9697ecf4cada315551dfbe48d4e","listen_addr":"tcp://0.0.0.0:26656","network":"demo","version":"0.38.0-rc3","channels":"40202122233038606100","moniker":"test","other":{"tx_index":"on","rpc_address":"tcp://127.0.0.1:26657"}},"SyncInfo":{"latest_block_hash":"E562E7858A00FD06CF81688440BCF2DA343C7A6346EA6A2C4B84E6B54E59FDC7","latest_app_hash":"F0880C04E7D37A25A960156E37516BC31FADF056CAA1785A8DA736700BD68170","latest_block_height":"1314","latest_block_time":"2023-07-29T11:07:02.629519262Z","earliest_block_hash":"C03B16E09172B09C51EE1CEDC48675918504B7DCA5F0EB56610F45168F110BB8","earliest_app_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","earliest_block_height":"1","earliest_block_time":"2023-07-29T09:17:05.480606618Z","catching_up":false},"ValidatorInfo":{"Address":"Xht+QS08KDJnvvaU1RFeBF+nEUI=","PubKey":{"type":"tendermint/PubKeyEd25519","value":"zYJRQ/WvnTy4u7P/nYTLSMHw2xdzjrga7wsYRFVAjrg="},"VotingPower":"1"}}

This PR:

{"node_info":{"protocol_version":{"p2p":"8","block":"11","app":"0"},"id":"af727ae22dedb9697ecf4cada315551dfbe48d4e","listen_addr":"tcp://0.0.0.0:26656","network":"demo","version":"0.38.0-rc3","channels":"40202122233038606100","moniker":"test","other":{"tx_index":"on","rpc_address":"tcp://127.0.0.1:26657"}},"sync_info":{"latest_block_hash":"EFCF39C80880666BFDCE46F89951D76E127819905B01C74D5CB4B3ACF71F99C9","latest_app_hash":"6D6495C600B32FE1B9B9E6B19D95757F7942AB87CC6E62C21EBF770752C39160","latest_block_height":"1312","latest_block_time":"2023-07-29T11:06:52.59880331Z","earliest_block_hash":"C03B16E09172B09C51EE1CEDC48675918504B7DCA5F0EB56610F45168F110BB8","earliest_app_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","earliest_block_height":"1","earliest_block_time":"2023-07-29T09:17:05.480606618Z","catching_up":false},"validator_info":{"address":"5E1B7E412D3C283267BEF694D5115E045FA71142","pub_key":{"type":"tendermint/PubKeyEd25519","value":"zYJRQ/WvnTy4u7P/nYTLSMHw2xdzjrga7wsYRFVAjrg="},"voting_power":"1"}}

At least it is coherent with CometBFT status endpoint.

@julienrbrt julienrbrt requested review from tac0turtle and removed request for tac0turtle July 29, 2023 11:17
@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Jul 29, 2023
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default output is still YAML, we should revert back to JSON. Still need a changelog as well.

client/rpc/status.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@julienrbrt julienrbrt self-assigned this Jul 30, 2023
@julienrbrt julienrbrt added this pull request to the merge queue Aug 1, 2023
Merged via the queue into cosmos:main with commit 31f7247 Aug 1, 2023
mergify bot pushed a commit that referenced this pull request Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release C:CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants