JSON & YAML output for kubectl api-resources #126338
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @dharmit! |
|
Hi @dharmit. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Unit tests fail like below: |
|
@dharmit I'll take a closer look at this PR later, but to answer your question about the failing unit test, it appears to be due to Adding this before |
|
/ok-to-test |
There was a problem hiding this comment.
Hi @dharmit I left a few comments for you to look into.
Ping me if you need clarification or if you make changes and want me to take another look.
staging/src/k8s.io/kubectl/pkg/cmd/apiresources/apiresources_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/kubectl/pkg/cmd/apiresources/apiresources_printer.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/kubectl/pkg/cmd/apiresources/apiresources_test.go
Outdated
Show resolved
Hide resolved
| name += "." + r.APIGroup | ||
| } | ||
| if _, err := fmt.Fprintf(writer, "%s\n", name); err != nil { | ||
| //errs = append(errs, err) |
There was a problem hiding this comment.
In the original code, errors were collected and an aggregate error was returned.
Can this code do the same thing as the original code, in order to preserve the existing behavior?
There was a problem hiding this comment.
This is something I wanted an opinion on, so I left this part commented instead of removing it.
The only way I could think of preserving the existing behaviour was by introducing a goroutine and using an error channel to put each error on. Would that be reasonable? Or does it sound like over-engineering?
There was a problem hiding this comment.
I don't think it should need channels.
It should be able to return an aggregate error, something like this:
if len(errs) > 0 {
return errors.NewAggregate(errs)
}
| } | ||
| for _, r := range lists { | ||
| rl.APIResources = append(rl.APIResources, r.APIResources...) | ||
| } | ||
|
|
||
| for _, list := range lists { |
There was a problem hiding this comment.
Shouldn't this entire for loop be contained within APIResourcesPrinter?
It only applies to when you are not printing JSON or YAML, right?
Then I think that solves the resources global variable problem, because resources can remain a local variable inside APIResourcesPrinter now, if I am understanding this correctly.
There was a problem hiding this comment.
This is actually the part that took me the longest while working on this issue. So I hope I understood things correctly.
So lists populated by the call
lists, err := o.discoveryClient.ServerPreferredResources()is a slice of type metav1.APIResourceList. Initially, I intended to use it directly in the respective PrintObj methods. But while metav1.APIResourceList itself implements the runtime.Object interface, a slice of it doesn't. Hence, it is how you see it right now.
I could possibly be missing something here. So please correct me if I am wrong. :)
There was a problem hiding this comment.
OK, I think I understand the reason.
You're re-purposing metav1.APIResourceList so that you have something that implements runtime.Object for the printer interface to work.
However, this doesn't seem like a good idea to me. It is really a list of lists, so merging them all into a single APIResourceList is losing some necessary details. For example, I notice in the unit test for json, it is missing the group version (v1 for Foo and Bar, and somegroup/v1 for Baz and NoVerbs).
| return err | ||
| } | ||
| } | ||
|
|
||
| sort.Stable(sortableResource{resources, o.SortBy}) |
There was a problem hiding this comment.
Similar to my previous comment, I think this can be moved inside APIResourcesPrinter because it does not apply when you are printing JSON or YAML.
There was a problem hiding this comment.
I think we need to keep it here, since there's no straightforward way of accessing the o.SortBy within APIResourcePrinter as it is right now. Of course, we could modify APIResourcePrinter to also include the sortBy field and then this could go there. I'll do as you suggest.
There was a problem hiding this comment.
Yeah, I think sorting would be an implementation detail of the printer, so it makes sense to have a sortBy field on the printer.
There was a problem hiding this comment.
I think, there is no need sorting for json and yaml. We can add validation to not allow this and sorting can be moved under custom (default) printer.
@brianpursley thanks a lot for your feedback. 🙏🏽
I have made a few changes and have a few questions for things I haven't changed. Your responses to how I could approach this would be greatly helpful. |
|
OK, it looks like the problem is that you've got I'm trying to think of a good way to get around this situation... 🤔 |
|
I wonder if it even makes sense to try to use the Some other commands have alternate printers, like I wonder if this command could have its own printer that doesn't require a Check out metrics_printer.go which |
I get your point about trying to make something fit that's not an exact fit. What's also messed up in the current JSON output is that
Neither of these have JSON output. OTOH, I'm literally brainstorming here so that I don't end up going down a rabbit hole, because I spent a few weeks before opening this PR. 😉 Also, @ardaguclu WDYT? Pinging you because you reviewed an earlier PR for this feature - #116926. |
| @@ -58,22 +59,25 @@ var ( | ||
| kubectl api-resources --api-group=rbac.authorization.k8s.io`) | ||
| ) | ||
|
|
||
| var resources []groupResource |
There was a problem hiding this comment.
This makes everything harder, there shouldn't be any global variable for this.
| @@ -151,6 +156,23 @@ func (o *APIResourceOptions) Complete(restClientGetter genericclioptions.RESTCli | ||
| o.groupChanged = cmd.Flags().Changed("api-group") | ||
| o.nsChanged = cmd.Flags().Changed("namespaced") | ||
|
|
||
| var printer printers.ResourcePrinter | ||
| if o.PrintFlags.OutputFormat != nil && len(*o.PrintFlags.OutputFormat) > 0 { |
There was a problem hiding this comment.
The logic in this function looks correct.
| return err | ||
| } | ||
| } | ||
|
|
||
| sort.Stable(sortableResource{resources, o.SortBy}) |
There was a problem hiding this comment.
I think, there is no need sorting for json and yaml. We can add validation to not allow this and sorting can be moved under custom (default) printer.
| noHeaders bool | ||
| } | ||
|
|
||
| type PrintFlags struct { |
There was a problem hiding this comment.
No need to have PrintFlags in here, built-in PrintFlags already achieves this for us.
| OutputFormat *string | ||
| } | ||
|
|
||
| func (f *PrintFlags) AllowedFormats() []string { |
There was a problem hiding this comment.
No need to have AllowedFormats in here and probably the reason of the usage of JSONYamlPrintFlags. We can manually check this in the code.
| return p, nil | ||
| } | ||
|
|
||
| func (f *PrintFlags) AddFlags(cmd *cobra.Command) { |
| `, | ||
| expectedInvalidations: 1, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Thank you for this unit test. But we definitely need more.
@ardaguclu I have added unit tests for JSON & YAML output and also one more that I thought was relevant to cover. PTAL. Please also suggest me if more unit test needs to be added. I couldn't think of more. As for integration tests, can you point me where I should look for example tests for |
|
@ardaguclu I have updated the PR based on your feedback and added unit tests. Just want to point out that @brianpursley mentioned in the past that the JSON/YAML outputs should preferably have that. However, we're creating that groupVersion info here. I don't see how we could have it in the current approach of flattening the Otherwise, I think, this PR is ready for review. |
|
Regarding the failing linter hints. The first error is: We're checking And the other three errors are with unit tests: But the error return value of other test cases is also not being checked, AFAICS. EDIT: Just noticed similar linter errors for other PRs too (#125489, #127341). What am I missing here? I'll change the first one from What's also confusing me is that both Also, what is the meaning of |
| } | ||
| } | ||
|
|
||
| if o.NoHeaders == false && o.Output != "name" { | ||
| if err = printContextHeaders(w, o.Output); err != nil { | ||
| if o.NoHeaders == false && *o.PrintFlags.OutputFormat == "" || (*o.PrintFlags.OutputFormat == "wide" && !o.NoHeaders) { |
There was a problem hiding this comment.
| if o.NoHeaders == false && *o.PrintFlags.OutputFormat == "" || (*o.PrintFlags.OutputFormat == "wide" && !o.NoHeaders) { | |
| if !o.NoHeaders && (*o.PrintFlags.OutputFormat == "" || *o.PrintFlags.OutputFormat == "wide") { |
There was a problem hiding this comment.
Thanks. My local branch had the !o.NoHeaders part of it already, but not the correct OR check that you provided. Done.
|
In the unit tests I do not see API group and version. Those are part of the existing output formats, and should be present in the JSON and YAML formats too. |
To generate the JSON and YAML outputs, we are flattening the slice of We are flattening it because each element of that slice contains a slice of Now, the From a user perspective, I understand that this might not be ideal output. But I can't think of a way here without creating a new struct that also implements the kubernetes/staging/src/k8s.io/cli-runtime/pkg/printers/interface.go Lines 34 to 38 in 6cc3570 PS: I don't expect to have done a good job at explaining the dilemma here, but I'm willing to make changes that seem necessary here. @ardaguclu could you please chime in? :) |
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
|
@dharmit: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
After reviewing the entire PR I'm more convinced that rather than using entire PrintFlags struct, you should only add json/yaml printer, since you're not using anything else other than just those two printers. Even more, rather than wiring the flags structure, I'd advise you to just inject the yaml/json printer directly based on the --output flag value provided by the user. I think this should simplify your implementation a bit.
| supportedOutputTypes := sets.New[string]("", "wide", "name") | ||
| if !supportedOutputTypes.Has(o.Output) { | ||
| return fmt.Errorf("--output %v is not available", o.Output) | ||
| supportedOutputTypes := sets.NewString("", "wide", "name", "json", "yaml") |
There was a problem hiding this comment.
You've decided to use entire PrintFlags which includes json/yaml printer, name printer and also template printer. Each one of them adds its own flags, but also has the ability to validate the contents of --output flag. With this check you're basically eliminating that last printer which I'm not sure we want here in the first place.
So my question for you is, which approach you prefer to have all of the printers (including the templating ones as well) or only the json/yaml and name printers? In either case this check is not necessary, since when creating the printer you'll get appropriate error if --output flag is not one of the accepted ones.
There was a problem hiding this comment.
So my question for you is, which approach you prefer to have all of the printers (including the templating ones as well) or only the json/yaml and name printers?
Based on what you mentioned, I'd go with only the JSON/YAML printers and skip the rest.
In either case this check is not necessary, since when creating the printer you'll get appropriate error if --output flag is not one of the accepted ones.
But there already was a check for sets.New[string]("", "wide", "name"). Don't we need these? If no, how do we validate this?
Besides, we don't have OutputFormat field any more if we go with JSONYamlPrintFlags. What I can't wrap my head around is how do we make the user choose from these options:
- wide
- name
- json
- yaml
The reason I can't wrap my head around it is that while user can do -o wide and -o name, these are not delegated to any of the printers in staging/src/k8s.io/cli-runtime/pkg/printers, but implemented using switch..case:
kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/apiresources/apiresources.go
Lines 219 to 239 in fd15e3f
There was a problem hiding this comment.
Based on what you mentioned, I'd go with only the JSON/YAML printers and skip the rest.
Sounds about right.
But there already was a check for
sets.New[string]("", "wide", "name"). Don't we need these? If no, how do we validate this?
See my above answer.
Besides, we don't have
OutputFormatfield any more if we go withJSONYamlPrintFlags. What I can't wrap my head around is how do we make the user choose from these options:
I believe your proposal from this comment answers that question. I really like your idea of doing something similar to get flags, where you'll be able to embed JSONYamlPrintFlags, NameFlags and most likely copy parts from get's HumanReadableFlags. This will allow you to get rid of this entire check, and rely on the o.PrintFlags.ToPrinter() invocation which will give you an error if someone tries to use an unknown output format. Hope that helps 😄
| var printer printers.ResourcePrinter | ||
| if o.PrintFlags.OutputFormat != nil && len(*o.PrintFlags.OutputFormat) > 0 && | ||
| (*o.PrintFlags.OutputFormat == "json" || *o.PrintFlags.OutputFormat == "yaml") { | ||
| printer, err = o.PrintFlags.ToPrinter() |
There was a problem hiding this comment.
Based on what I wrote above it's sufficient to squash this condition to just:
if o.PrintFlags.OutputFormat != nil {
printer, err = o.PrintFlags.ToPrinter()
...since the rest will be handled by the printers, including the validation I wrote above.
| @@ -205,18 +228,36 @@ func (o *APIResourceOptions) RunAPIResources() error { | ||
| APIGroupVersion: gv.String(), | ||
| APIResource: resource, | ||
| }) | ||
| apiResources = append(apiResources, resource) | ||
| } | ||
| if len(apiResources) > 0 { |
There was a problem hiding this comment.
Don't filter out empty groups, this will cause that you won't print groups that don't have any resources. Filtering is happening earlier (there are continue blocks ensuring you'll skip group you don't care about).
| @@ -171,6 +185,7 @@ func (o *APIResourceOptions) RunAPIResources() error { | ||
| } | ||
|
|
||
| resources := []groupResource{} | ||
| var filteredList []*metav1.APIResourceList |
There was a problem hiding this comment.
Can you rename the resources to allResources, so it's clear which one is what.
| } | ||
| } | ||
|
|
||
| if o.NoHeaders == false && o.Output != "name" { | ||
| if err = printContextHeaders(w, o.Output); err != nil { | ||
| if !o.NoHeaders && (*o.PrintFlags.OutputFormat == "" || *o.PrintFlags.OutputFormat == "wide") { |
There was a problem hiding this comment.
| if !o.NoHeaders && (*o.PrintFlags.OutputFormat == "" || *o.PrintFlags.OutputFormat == "wide") { | |
| if !o.NoHeaders && (o.PrintFlags.OutputFormat == nil || *o.PrintFlags.OutputFormat == "" || *o.PrintFlags.OutputFormat == "wide") { |
| return err | ||
| } | ||
| } | ||
|
|
||
| if *o.PrintFlags.OutputFormat == "json" || *o.PrintFlags.OutputFormat == "yaml" { | ||
| flatList := &metav1.APIResourceList{ |
There was a problem hiding this comment.
What is the reason you can't create this flatList already when creating filteredList? With appropriate comment above that variable.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dharmit, hashim21223445 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
/remove-lifecycle stale Will get onto this soon. Still very much want to get this merged. |
|
@soltysh besides asking this question (#126338 (comment)) I have pushed a separate branch which contains this commit dharmit@52f7ccf. It uses a separate Let me know what you think. |
I really like your approach! I think the only suggestion I have is to add something similar to HumanReadableFlags to handle both the name and wide options. Just to make it more composable. |
|
@soltysh Adding the HumanReadableFlags and NamePrintFlags to the mix is causing an issue with the Can you help me figure what I'm doing wrong or what I could try? I have pushed the code as a separate commit dharmit@4232182. |
As mentioned on slack:
I haven't looked too closely into what kind of error you're getting, but I'm suspecting this is coming from that. Ideally I'd suggest either opening entirely new PR or update the existing one with that code and let's continue discussion there. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
JSON & YAML output for kubectl api-resources
Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#1382
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: