[WIP] Move exposed Printing funcs to cmd/util/factory_builder.go #50113

juanvallejo · 2017-08-03T22:57:37Z

Proposal: #50388

Related doc: https://docs.google.com/a/redhat.com/document/d/15uieHE_QnfF9eEj-AAbAvo3xBqfyu5x1v5Cghzy4ZDI/edit?usp=sharing

Related cards:

Release note:

NONE

This PR attempts to only expose printing methods through the cmdutil factory.
It also attempts to remove complexity from using these printing methods by consolidating printers.OutputOptions and printers.PrintOptions into a single struct.

It also attempts remove cobra.Command dependency from printer methods (except PrinterForCommand), and attempts to facilitate code reusability by consumers of factory printing methods.

cc @fabianofranz @kubernetes/sig-cli @deads2k

k8s-ci-robot · 2017-08-03T22:57:44Z

Hi @juanvallejo. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-08-03T22:57:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juanvallejo
We suggest the following additional approver: dchen1107

Assign the PR to them by writing /assign @dchen1107 in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

juanvallejo · 2017-08-03T23:02:31Z

pkg/kubectl/cmd/util/factory.go

@@ -149,6 +149,7 @@ type ClientAccessFactory interface {
 	// SuggestedPodTemplateResources returns a list of resource types that declare a pod template
 	SuggestedPodTemplateResources() []schema.GroupResource

+	// TODO: how is this different than PrinterForMapping?
 	// Returns a Printer for formatting objects of the given type or an error.
 	Printer(mapping *meta.RESTMapping, options printers.PrintOptions) (printers.ResourcePrinter, error)


@fabianofranz maybe we could get rid of this method and use PrinterForMapping instead?

juanvallejo · 2017-08-08T22:26:19Z

pkg/kubectl/cmd/util/factory_client_access.go

 	return p, nil
 }

+// TODO(juanvallejo): change printersinternal.AddHandlers to printersinternal.GetDefaultHandlers and return
+// a *printers.HandlerEntry list. Handle any errors returned from generating this list.


@fabianofranz working on a patch for this. Will expose pkg/printers.handlerEntry, and update pkg/printers/internalversion.AddHandlers to return a slice of *pkg/printers.HandlerEntry.

This will allow a new factory method that returns the list of client-side registered printer handlers, per the doc on printer refactor.

Your thoughts and feedback are welcome on this

smarterclayton · 2017-08-09T01:12:24Z

What doc on printer refactor?

…

On Tue, Aug 8, 2017 at 6:27 PM, Juan Vallejo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/kubectl/cmd/util/factory_client_access.go <#50113 (comment)> : > return p, nil } +// TODO(juanvallejo): change printersinternal.AddHandlers to printersinternal.GetDefaultHandlers and return +// a *printers.HandlerEntry list. Handle any errors returned from generating this list. @fabianofranz <https://github.com/fabianofranz> working on a separate PR for this. Will expose pkg/printers.handlerEntry <https://github.com/kubernetes/kubernetes/blob/master/pkg/printers/humanreadable.go#L48>, and update pkg/printers/internalversion.AddHandlers <https://github.com/kubernetes/kubernetes/blob/master/pkg/printers/internalversion/printers.go#L67> to return a slice of *pkg/printers#HandlerEntry. This will allow a new factory method that returns the list of client-side registered printer handlers, per the doc on printer refactor <https://docs.google.com/document/d/15uieHE_QnfF9eEj-AAbAvo3xBqfyu5x1v5Cghzy4ZDI/edit?ts=5978e6a6#heading=h.1l1gx9w54z6p> . Your thoughts and feedback are welcome on this — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#50113 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p8N8Y8WjX45gwCBZx789jGFIKf3Lks5sWOC9gaJpZM4OtFl-> .

smarterclayton · 2017-08-09T01:12:28Z

Linked doc is not available to members of sig-cli?

…

On Tue, Aug 8, 2017 at 9:12 PM, Clayton Coleman ***@***.***> wrote: What doc on printer refactor? On Tue, Aug 8, 2017 at 6:27 PM, Juan Vallejo ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In pkg/kubectl/cmd/util/factory_client_access.go > <#50113 (comment)> > : > > > return p, nil > } > > +// TODO(juanvallejo): change printersinternal.AddHandlers to printersinternal.GetDefaultHandlers and return > +// a *printers.HandlerEntry list. Handle any errors returned from generating this list. > > @fabianofranz <https://github.com/fabianofranz> working on a separate PR > for this. Will expose pkg/printers.handlerEntry > <https://github.com/kubernetes/kubernetes/blob/master/pkg/printers/humanreadable.go#L48>, > and update pkg/printers/internalversion.AddHandlers > <https://github.com/kubernetes/kubernetes/blob/master/pkg/printers/internalversion/printers.go#L67> > to return a slice of *pkg/printers#HandlerEntry. > > This will allow a new factory method that returns the list of client-side > registered printer handlers, per the doc on printer refactor > <https://docs.google.com/document/d/15uieHE_QnfF9eEj-AAbAvo3xBqfyu5x1v5Cghzy4ZDI/edit?ts=5978e6a6#heading=h.1l1gx9w54z6p> > . > > Your thoughts and feedback are welcome on this > > — > You are receiving this because you were assigned. > Reply to this email directly, view it on GitHub > <#50113 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABG_p8N8Y8WjX45gwCBZx789jGFIKf3Lks5sWOC9gaJpZM4OtFl-> > . >

fabianofranz · 2017-08-09T15:37:19Z

@kubernetes/sig-cli-misc @deads2k @liggitt @sttts

juanvallejo · 2017-08-09T18:02:44Z

Link to proposal: #50388

…tion

shiywang · 2017-08-11T01:08:41Z

/ok-to-test

shiywang · 2017-08-11T09:07:16Z

pkg/printers/printers.go

-	}
-
-	format, formatArgument, allowMissingTemplateKeys := outputOpts.FmtType, outputOpts.FmtArg, outputOpts.AllowMissingKeys
+func GetStandardPrinter(noHeaders bool, mapper meta.RESTMapper, typer runtime.ObjectTyper, encoder runtime.Encoder, decoders []runtime.Decoder, options PrintOptions) (ResourcePrinter, error) {


as we removed PrinterForCommand, could we also remove noHeaders bool ?

shiywang · 2017-08-11T09:27:29Z

pkg/kubectl/cmd/util/printing.go

+// printerWithOptions returns the printer for the given options.
+// Requires that printer flags have been added to cmd (see AddPrinterFlags).
+func printerWithOptions(mapper meta.RESTMapper, typer runtime.ObjectTyper, encoder runtime.Encoder, decoders []runtime.Decoder, options printers.PrintOptions) (printers.ResourcePrinter, error) {
+	printer, err := printers.GetStandardPrinter(options.NoHeaders, mapper, typer, encoder, decoders, options)


options.NoHeaders is included in options right ? could we only use options ?

k8s-github-robot · 2017-08-15T21:35:47Z

@juanvallejo PR needs rebase

k8s-ci-robot · 2017-08-17T19:53:27Z

@juanvallejo: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	`be00aa2`	link	`/test pull-kubernetes-bazel`
pull-kubernetes-unit	`be00aa2`	link	`/test pull-kubernetes-unit`
pull-kubernetes-node-e2e	`be00aa2`	link	`/test pull-kubernetes-node-e2e`
pull-kubernetes-federation-e2e-gce	`be00aa2`	link	`/test pull-kubernetes-federation-e2e-gce`
pull-kubernetes-kubemark-e2e-gce	`be00aa2`	link	`/test pull-kubernetes-kubemark-e2e-gce`
pull-kubernetes-e2e-kops-aws	`be00aa2`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-verify	`be00aa2`	link	`/test pull-kubernetes-verify`
pull-kubernetes-e2e-gce-etcd3	`be00aa2`	link	`/test pull-kubernetes-e2e-gce-etcd3`

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/test-infra repository. I understand the commands that are listed here.

juanvallejo · 2017-09-18T17:28:40Z

/assign seans3
/assign mengqiy

k8s-ci-robot · 2017-09-18T17:28:41Z

@juanvallejo: GitHub didn't allow me to assign the following users: seans3.

Note that only kubernetes members can be assigned.

In response to this:

/assign seans3
/assign mengqiy

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.

juanvallejo · 2017-10-30T20:04:15Z

Splitting this PR into multiple easier-to-review PRs.
Tracking commits that have been split so far in this comment:

f4d7174 - split into separate PR: consolidate printer OutputOpts w PrintOpts #54811
8428ac0 - split into separate PR: move cmd/util/printing.go#PrintSuccess to factory_builder.go #54867
5b68929 - split into separate PR: move cmd/util/printing.go#PrintResourceInfoForCommand -> factory… #54877
83198aa - split into separate PR: ensure PrinterForCommand is consumed through cmdutil.Factory #54881

@fabianofranz

…intResourceInfoRorCommand-cmdutil-factory Automatic merge from submit-queue (batch tested with PRs 54602, 54877, 55243, 55509, 55128). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. move cmd/util/printing.go#PrintResourceInfoForCommand -> factory… **Release note**: ```release-note NONE ``` This patch is one in a series of patches that aims to move all printing functions to the `cmdutil.Factory` in order to make the factory the one-stop shop for accessing printers in the client. This PR is related to kubernetes#50113 and aims to break the set of changes introduced in this commit in order to make them easier to review. @fabianofranz @mengqiy @shiywang @seans3

@fabianofranz

…s-cmdutil-factory Automatic merge from submit-queue (batch tested with PRs 55233, 55927, 55903, 54867, 55940). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. move cmd/util/printing.go#PrintSuccess to factory_builder.go **Release note**: ```release-note NONE ``` This patch is one in a series of patches that aims to move all printing functions to the `cmdutil.Factory` in order to make the factory the one-stop shop for accessing printers in the client. This PR is related to #50113 and aims to break that set of changes by introducing a portion of them in this commit in order to make them easier to review. @fabianofranz @mengqiy @shiywang @seans3

@fabianofranz

…tOpts-printOpts Automatic merge from submit-queue (batch tested with PRs 54811, 54292, 56103). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. consolidate printer OutputOpts w PrintOpts This patch removes the use of printers.OutputOptions in favor of only having a single struct for setting / passing printer options set by user flags. This PR is related to #50113 and aims to break the set of changes introduced in [this commit](f4d7174) in order to make them easier to review. **Release note**: ```release-note NONE ``` cc @fabianofranz @mengqiy @shiywang @seans3

juanvallejo · 2017-11-27T15:50:11Z

Closing this in favor of:

@fabianofranz

…md-through-factory 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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. ensure PrinterForCommand is consumed through cmdutil.Factory **Release note**: ```release-note NONE ``` This patch is one in a series of patches that aims to move all printing functions to the `cmdutil.Factory` in order to make the 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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 3, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 3, 2017

k8s-github-robot assigned smarterclayton and dchen1107 Aug 3, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 3, 2017

juanvallejo commented Aug 3, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch from 51f943c to 8cbe4df Compare August 4, 2017 15:35

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2017

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch from d567602 to c06055d Compare August 4, 2017 20:42

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2017

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch from c06055d to 86e2410 Compare August 7, 2017 14:31

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 7, 2017

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch 2 times, most recently from 4e0da42 to f23a703 Compare August 8, 2017 20:05

juanvallejo mentioned this pull request Aug 8, 2017

Fix the kubectl printing stack openshift/origin#15280

Closed

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch from f23a703 to 1e76b7d Compare August 8, 2017 22:20

juanvallejo commented Aug 8, 2017

View reviewed changes

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Aug 9, 2017

juanvallejo mentioned this pull request Aug 9, 2017

expose printers.AddHandler through clientcmd Factory #50394

Closed

juanvallejo added 2 commits August 10, 2017 14:50

rename PrintHandler -> TabularPrintHandler; update printer type asser…

1fd4cb7

…tion

unexpose cmd/util/printing#PrinterForCommand

83198aa

shiywang reviewed Aug 11, 2017

View reviewed changes

remove cmdutil.Factory#DefaultResourceFilterOptions

d097d82

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 11, 2017

pass printers.PrintOptions by pointer

cc841e9

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch from f5f1607 to cc841e9 Compare August 11, 2017 22:27

update bazel

7e1b487

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch from d106430 to 7e1b487 Compare August 14, 2017 15:13

rename DecoratedPrinterWithOptions -> VersionedPrinterWithOptions

ca1df7a

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2017

combine VersionedPrinterWithOptions with PrinterWithOptions

be00aa2

juanvallejo force-pushed the jvallejo/limit-printer-exposure branch from 7266b47 to be00aa2 Compare August 17, 2017 19:11

k8s-ci-robot assigned mengqiy Sep 18, 2017

juanvallejo mentioned this pull request Oct 30, 2017

consolidate printer OutputOpts w PrintOpts #54811

Merged

This was referenced Oct 31, 2017

move cmd/util/printing.go#PrintSuccess to factory_builder.go #54867

Merged

move cmd/util/printing.go#PrintResourceInfoForCommand -> factory… #54877

Merged

ensure PrinterForCommand is consumed through cmdutil.Factory #54881

Merged

juanvallejo closed this Nov 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Move exposed Printing funcs to cmd/util/factory_builder.go #50113

[WIP] Move exposed Printing funcs to cmd/util/factory_builder.go #50113

juanvallejo commented Aug 3, 2017 •

edited

Loading

k8s-ci-robot commented Aug 3, 2017

k8s-github-robot commented Aug 3, 2017

juanvallejo Aug 3, 2017

juanvallejo Aug 8, 2017 •

edited

Loading

smarterclayton commented Aug 9, 2017 via email

smarterclayton commented Aug 9, 2017 via email

fabianofranz commented Aug 9, 2017

juanvallejo commented Aug 9, 2017

shiywang commented Aug 11, 2017

shiywang Aug 11, 2017

shiywang Aug 11, 2017 •

edited

Loading

k8s-github-robot commented Aug 15, 2017

k8s-ci-robot commented Aug 17, 2017

juanvallejo commented Sep 18, 2017

k8s-ci-robot commented Sep 18, 2017

juanvallejo commented Oct 30, 2017 •

edited

Loading

juanvallejo commented Nov 27, 2017

[WIP] Move exposed Printing funcs to cmd/util/factory_builder.go #50113

[WIP] Move exposed Printing funcs to cmd/util/factory_builder.go #50113

Conversation

juanvallejo commented Aug 3, 2017 • edited Loading

k8s-ci-robot commented Aug 3, 2017

k8s-github-robot commented Aug 3, 2017

juanvallejo Aug 3, 2017

Choose a reason for hiding this comment

juanvallejo Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

smarterclayton commented Aug 9, 2017 via email

smarterclayton commented Aug 9, 2017 via email

fabianofranz commented Aug 9, 2017

juanvallejo commented Aug 9, 2017

shiywang commented Aug 11, 2017

shiywang Aug 11, 2017

Choose a reason for hiding this comment

shiywang Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

k8s-github-robot commented Aug 15, 2017

k8s-ci-robot commented Aug 17, 2017

juanvallejo commented Sep 18, 2017

k8s-ci-robot commented Sep 18, 2017

juanvallejo commented Oct 30, 2017 • edited Loading

juanvallejo commented Nov 27, 2017

juanvallejo commented Aug 3, 2017 •

edited

Loading

juanvallejo Aug 8, 2017 •

edited

Loading

shiywang Aug 11, 2017 •

edited

Loading

juanvallejo commented Oct 30, 2017 •

edited

Loading