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 output mechanism for CLI commands #29

Merged
merged 9 commits into from
Jan 9, 2023

Conversation

RRethy
Copy link
Contributor

@RRethy RRethy commented Jan 9, 2023

Part of #3

This PR adds an output mechanism for CLI commands.

We had opt/ and output/ in place but I felt they were not going to be flexible enough to handle all possible printers, and things like HandleString belonged in a logger abstraction.

This adds an OutputFormat class which should be scoped to a subcommand and accepts a list of printers, it then configures the flags, handles validation, and passing along the object to the correct printer.

This style of --output is very similar to kubectl. By doing it like this we will be able to fairly easily handle output formats which allow multiple flags e.g..

Decisions made:

  1. Move non cmd/ go code into pkg/
  2. Output format flags are scoped and configured in the subcommand since some subcommands may permit different output formats
  3. Non formatted output (specifically logs) doesn't belong in OutputFormat
  4. We configure output with --output yaml rather than --yaml, matching kubectl

cc @voigt since you've made modifications in this code previously

@RRethy RRethy mentioned this pull request Jan 9, 2023
@voigt
Copy link
Collaborator

voigt commented Jan 9, 2023

We had opt/ and output/ in place but I felt they were not going to be flexible enough to handle all possible printers, and things like HandleString belonged in a logger abstraction.

Perfectly fine. Both were just another outcome of my cmd-structure PR. Also, HandleString came just out of necessity to handle the placeholders we had. Haven't put much thought into this :)

Move non cmd/ go code into pkg/

I like the decision. Now that we have a cli package what do you think about moving cmd into cli as well? This way we would have everything command related within ./pkg/cli. In #26 I was also working on some testing utils which could also fit into this dir. wdyt?

Like this:

$ tree -L 3 ./pkg
./pkg
└── cli
    ├── cmd
    │   ├── cmd.go
    │   ├── oci
    │   ├── root
    │   └── version
    ├── output_format.go
    ├── output_format_test.go
    └── printer
        ├── interface.go
        ├── json.go
        └── yaml.go

@dmah42
Copy link
Contributor

dmah42 commented Jan 9, 2023

why would each command have a different output format?

@RRethy
Copy link
Contributor Author

RRethy commented Jan 9, 2023

why would each command have a different output format?

If we take kubectl as an example, kubectl get supports a lot of output formats like json|yaml|jsonpath|go-template|etc while something like kubectl api-resources supports only short|wide. Looking at ae commands, I'm guessing the cert commands in #28 won't need the same output options as the observe or runtime subcommands.

That being said, after building out the subcommands a bit more, if we notice they'll all supporting the same --output formats then we can move this up to the root and use a default set of printers.

@RRethy
Copy link
Contributor Author

RRethy commented Jan 9, 2023

I like the decision. Now that we have a cli package what do you think about moving cmd into cli as well? This way we would have everything command related within ./pkg/cli. In #26 I was also working on some testing utils which could also fit into this dir. wdyt?

I'm good with that 👍

@krisnova krisnova merged commit 7618c6c into aurae-runtime:main Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants