ensure PrinterForCommand is consumed through cmdutil.Factory #54881
Conversation
k8s-ci-robot
added
release-note-none
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
juanvallejo
force-pushed
the
jvallejo/use-printer-for-cmd-through-factory
branch
3 times, most recently
from
fe55b6a
to
6f2ce18
Compare
|
/retest |
|
/test pull-kubernetes-unit |
|
/retest |
juanvallejo
force-pushed
the
jvallejo/use-printer-for-cmd-through-factory
branch
from
6f2ce18
to
784b2a1
Compare
|
/retest |
juanvallejo
force-pushed
the
jvallejo/use-printer-for-cmd-through-factory
branch
from
784b2a1
to
7cb7417
Compare
|
@kubernetes/sig-cli-maintainers /unassign |
|
ping @fabianofranz @mengqiy |
| @@ -131,7 +131,7 @@ func PrinterForCommand(cmd *cobra.Command, outputOpts *printers.OutputOptions, m | |||
| // we try to convert to HumanReadablePrinter, if return ok, it must be no generic | |||
| // we execute AddHandlers() here before maybeWrapSortingPrinter so that we don't | |||
| // need to convert to delegatePrinter again then invoke AddHandlers() | |||
| if humanReadablePrinter, ok := printer.(*printers.HumanReadablePrinter); ok { | |||
| if humanReadablePrinter, ok := printer.(printers.PrintHandler); ok { | |||
There was a problem hiding this comment.
entangling adding handlers and wrapping in this single function doesn't allow adding additional handlers in a calling factory. @smarterclayton, thoughts on the sorting wrapper being added this way?
There was a problem hiding this comment.
@liggitt will leave this line as-is for now. There is a proposal for discussing how to address adding handlers to a printer, however a goal would be to eliminate printer handlers altogether in favor of server-side printers.
ping @smarterclayton
k8s-github-robot
added
the
needs-rebase
juanvallejo
force-pushed
the
jvallejo/use-printer-for-cmd-through-factory
branch
from
7cb7417
to
8c9c2ee
Compare
k8s-github-robot
removed
the
needs-rebase
|
@mfojtik or @fabianofranz wondering if you could take a look? last PR left on https://trello.com/c/7wjtBnNu |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabianofranz, juanvallejo Associated issue requirement bypassed by: fabianofranz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
|
Automatic merge from submit-queue (batch tested with PRs 56676, 57050, 54881, 56822, 57113). If you want to cherry-pick this change to another branch, please follow the instructions here. |
Release note:
This patch is one in a series of patches that aims to move all
printing functions to the
cmdutil.Factoryin order to makethe factory the one-stop shop for accessing printers in the client.
This PR is related to #50113 and aims to break the set of changes
introduced in this commit in order to make them easier to review.
@fabianofranz @mengqiy @shiywang