Denote if a printer is generic. #46265
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @waseem. 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 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/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/L
|
@k8s-bot ok to test |
k8s-ci-robot
removed
the
needs-ok-to-test
| return nil, err | ||
| if humanReadablePrinter, ok := printer.(*printers.HumanReadablePrinter); ok { | ||
| printersinternal.AddHandlers(humanReadablePrinter) | ||
| printer = printers.ResourcePrinter(humanReadablePrinter) |
There was a problem hiding this comment.
Why is this line needed ? (may be a comment explaining the above two lines will help the reader here).
There was a problem hiding this comment.
@droot I added a comment explaining the need of these lines. While writing the comment I realized that I was doing an unnecessary conversion which I removed. PTAL.
Can you please queue the test runs again and also remove the release-note-label-needed label?
Thanks!
There was a problem hiding this comment.
Thanks for adding the comment. It looks good now. As per test-run and removing the label, @mengqiy took care of it.
|
|
||
| func shouldGetNewPrinterForMapping(printer printers.ResourcePrinter, lastMapping, mapping *meta.RESTMapping) bool { | ||
| return printer == nil || lastMapping == nil || mapping == nil || mapping.Resource != lastMapping.Resource | ||
| } |
There was a problem hiding this comment.
+1 to this change
|
LGTM |
|
@k8s-bot ok to test |
k8s-ci-robot
added
release-note-none
| case "wide": | ||
| fallthrough | ||
| case "": | ||
| return nil, false, nil | ||
|
|
||
| printer = NewHumanReadablePrinter(encoder, decoders[0], options) |
There was a problem hiding this comment.
Do we need to move this line up for wide only?
I suspect it changes the behavior when not specify --output flag.
There was a problem hiding this comment.
@mengqiy I don't follow how that is possible.
There are four general cases for --output flag.
- It's one of the generic formats these are
json,yaml,name,template/go-template,templatefile/go-templatefile,jsonpath,jsonpath-file,custom-columns, orcustom-columns-file. - A non-generic format
wideor"". - An unknown format.
In the context of just this function, wide and "" are treated the same because of the blank fallthrough in the case of wide. We could also have used just case "wide", "": to get the same result.
Now in case --output is actually set to "", it is actually set to template here and then it is used to retrieve the printer.
Should we get another pair of eyes for this?
|
/assign @fabianofranz |
|
/unassign |
|
@waseem mind squashing your commits? |
This fixes kubernetes#38779. This allows us to avoid case in which printers.GetStandardPrinter returns nil for both printer and err removing any potential panics that may arise throughout kubectl commands. Please see kubernetes#38779 and kubernetes#38112 for complete context. Add comment explaining adding handlers to printers.HumanReadablePrinter also remove an unnecessary conversion of printers.HumanReadablePrinter to printers.ResourcePrinter.
waseem
force-pushed
the
printers-genericity
branch
from
20294ae
to
8442a11
Compare
|
@fabianofranz I squashed the commits and rebased the branch. |
|
@waseem thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabianofranz, waseem
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
|
Automatic merge from submit-queue (batch tested with PRs 41563, 45251, 46265, 46462, 46721) |
Automatic merge from submit-queue Fix sort-by output problem Fixes kubernetes/kubectl#43 This bug was original introduced in pr here: #46265, I think next time if we touch something printer related package, maybe should let @smarterclayton have a review, although he is pretty busy I guess : ) and that package also changed a lot recently since he's been working on refactoring. this is a quick and dirty fix, not sure if there's better way, I will add some regression test soon... @kubernetes/sig-cli-pr-reviews ```release-note NONE ``` /assign @mengqiy /assign @smarterclayton
This fixes #38779.
This allows us to avoid case in which printers.GetStandardPrinter
returns nil for both printer and err removing any potential panics that
may arise throughout kubectl commands.
Please see #38779 and #38112 for complete context.