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

[PoC] Add --json flag to cli commands #3249

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joaopapereira
Copy link
Contributor

Description of the Change

This change adds the --json flags to the commands apps, app, and get-health-check commands as a PoC for us to understand how complicated this would be.

Copy link
Member

@a-b a-b left a comment

Choose a reason for hiding this comment

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

While it is debatable whether JSON belongs to UI, this is a good proof of concept.

@Gerg
Copy link
Member

Gerg commented Nov 19, 2024

I haven't reviewed this PR in detail, but a couple quick thoughts:

  1. Is there a way we can handle this flag globally, rather than needing to implement it as a flag for every command? Similar to the -v flag or the environment-variable-based configuration.
  2. What about --output=json instead of --json (inspired by AWS CLI)?

@joaopapereira
Copy link
Contributor Author

  1. Is there a way we can handle this flag globally, rather than needing to implement it as a flag for every command? Similar to the -v flag or the environment-variable-based configuration.

That was my first attempt but even when you look at the apps command you can see that the output is split into 3 different calls to the UI Writer, which makes it complicated to abstract out.
Another thing that I noticed is that in some cases changing the name of the keys, in the struct they are called "Name" but in the output we set it to "SpaceName" we also do translations which complicates even further the code.

  1. What about --output=json instead of --json (inspired by AWS CLI)?

I do not have a strong opinion about that, are we planning on adding TOML or YAML support, or are we just covering our basis here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants