Get command uses print-column extn from Openapi schema #46235

droot · 2017-05-22T18:31:31Z

What this PR does / why we need it:

Kubectl Get command now uses metadata 'x-kubernetes-print-column' from Openapi schema to display a resource. This is to enable richer experience for non-compiled types (like service catalog API resources) in Kubectl. This functionality is currently guarded by a boolean flag "use-openapi-print-columns".

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
fixes kubernetes/kubectl#22

Special notes for your reviewer:

Release note:

Get command uses OpenAPI schema to enhance display for a resource if run with flag 'use-openapi-print-columns'. 
An example command:
kubectl get pods --use-openapi-print-columns

k8s-ci-robot · 2017-05-22T18:31:33Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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 · 2017-05-22T18:31:39Z

Hi @droot. 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 @k8s-bot 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.

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.

pwittrock · 2017-05-22T22:16:05Z

pkg/api/v1/types.go

@@ -2568,6 +2568,7 @@ type PodStatusResult struct {

 // Pod is a collection of containers that can run on a host. This resource is created
 // by clients and scheduled onto hosts.
+// +k8s:openapi-gen=x-kubernetes-print-columns:custom-columns=NAME:.metadata.name,RSRC:.metadata.resourceVersion


Good for testing, but we don't want to merge this.

Yes, took it out.

pwittrock · 2017-05-22T22:18:26Z

pkg/kubectl/cmd/get.go

@@ -139,6 +139,8 @@ func NewCmdGet(f cmdutil.Factory, out io.Writer, errOut io.Writer) *cobra.Comman
 // RunGet implements the generic Get command
 // TODO: convert all direct flag accessors to a struct and pass that instead of cmd
 func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args []string, options *GetOptions) error {
+	go f.OpenAPISchema(cmdutil.GetOpenAPICacheDir(cmd))


lets flag guard this, default off until Antoine's optimization is merged

What happens to this if the command exits before this is finished? Should we defer waiting for this to finish or something?

Flag guarding is a good idea. I realized we will have to flag guard in other places as well.

About command exiting before this is finished, can potentially cause issues. We will have to fool proof OpenAPI cache reading/writing logic (basically making them idempotent in case of errors). For ex. we may fail while updating the cache (can happen in this case if we don't wait or crash in middle). Will discuss with Antoine about it.

Where does the result of this function go? Why is this running asynchronously? How do we not care about the fetched resource?

This is to trigger eager-loading of OpenAPI schema in cache. This is running asynchronously because we don't want to block the processing of Get command because OpenAPI cache may or may for the Get command processing.

It at least deserves a comment.

pwittrock · 2017-05-22T22:21:34Z

pkg/kubectl/cmd/util/factory_builder.go

+		// default specification to print this resource type
+		outputFormatFromOpenAPI := f.lookupDisplayColumnsFromOpenAPI(cmd, mapping)
+		if outputFormatFromOpenAPI != "" {
+			outputOpts = f.getOutputOptsFromOpenAPISpec(outputFormatFromOpenAPI)


should this line be wrapped into: lookupDisplayColumnsFromOpenAPI

moved this piece of code inside RunGet in get.go because printerForMapping should not deal with the business logic of output format for a mapping. That details belongs in Get command handling.

About your comment, pl. take a look at the new code and see if that looks OK.

pwittrock · 2017-05-22T22:22:52Z

pkg/kubectl/cmd/util/printing.go

@@ -162,6 +142,48 @@ func PrintResourceInfoForCommand(cmd *cobra.Command, info *resource.Info, f Fact
 	return printer.PrintObj(info.Object, out)
 }

+func extractOutputOptions(cmd *cobra.Command) *printers.OutputOptions {


much better. add some documentation to the function before merge plz

pwittrock · 2017-05-22T22:23:26Z

pkg/printers/customcolumn.go

@@ -153,6 +153,7 @@ type CustomColumnsPrinter struct {
 	Columns   []Column
 	Decoder   runtime.Decoder
 	NoHeaders bool
+	lastType  reflect.Type


what is this and why is it needed? (add comment)

Added comment. lastType records the type of resource printed last. This is to avoid repeating headers while printing same types of resources. So header gets printed only when type changes.

pwittrock · 2017-05-22T22:25:35Z

generally looks good. will want to add some tests.

pwittrock · 2017-05-22T22:25:50Z

/assign mengqiy

droot

Thanks for the review @pwittrock . Addressed the comments. PTAL.

droot · 2017-05-24T19:21:55Z

pkg/printers/customcolumn.go

@@ -153,6 +153,7 @@ type CustomColumnsPrinter struct {
 	Columns   []Column
 	Decoder   runtime.Decoder
 	NoHeaders bool
+	lastType  reflect.Type


Added comment. lastType records the type of resource printed last. This is to avoid repeating headers while printing same types of resources. So header gets printed only when type changes.

droot · 2017-05-24T19:22:27Z

pkg/kubectl/cmd/util/printing.go

@@ -162,6 +142,48 @@ func PrintResourceInfoForCommand(cmd *cobra.Command, info *resource.Info, f Fact
 	return printer.PrintObj(info.Object, out)
 }

+func extractOutputOptions(cmd *cobra.Command) *printers.OutputOptions {


droot · 2017-05-24T19:26:19Z

pkg/kubectl/cmd/util/factory_builder.go

+		// default specification to print this resource type
+		outputFormatFromOpenAPI := f.lookupDisplayColumnsFromOpenAPI(cmd, mapping)
+		if outputFormatFromOpenAPI != "" {
+			outputOpts = f.getOutputOptsFromOpenAPISpec(outputFormatFromOpenAPI)


moved this piece of code inside RunGet in get.go because printerForMapping should not deal with the business logic of output format for a mapping. That details belongs in Get command handling.

About your comment, pl. take a look at the new code and see if that looks OK.

droot · 2017-05-24T19:38:42Z

pkg/kubectl/cmd/get.go

@@ -139,6 +139,8 @@ func NewCmdGet(f cmdutil.Factory, out io.Writer, errOut io.Writer) *cobra.Comman
 // RunGet implements the generic Get command
 // TODO: convert all direct flag accessors to a struct and pass that instead of cmd
 func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args []string, options *GetOptions) error {
+	go f.OpenAPISchema(cmdutil.GetOpenAPICacheDir(cmd))


Flag guarding is a good idea. I realized we will have to flag guard in other places as well.

About command exiting before this is finished, can potentially cause issues. We will have to fool proof OpenAPI cache reading/writing logic (basically making them idempotent in case of errors). For ex. we may fail while updating the cache (can happen in this case if we don't wait or crash in middle). Will discuss with Antoine about it.

droot · 2017-05-24T19:39:01Z

pkg/api/v1/types.go

@@ -2568,6 +2568,7 @@ type PodStatusResult struct {

 // Pod is a collection of containers that can run on a host. This resource is created
 // by clients and scheduled onto hosts.
+// +k8s:openapi-gen=x-kubernetes-print-columns:custom-columns=NAME:.metadata.name,RSRC:.metadata.resourceVersion


Yes, took it out.

alexandercampbell · 2017-05-24T20:52:11Z

pkg/kubectl/cmd/get.go

@@ -501,7 +513,9 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 			}
 			continue
 		}
-		if err := printer.PrintObj(decodedObj, w); err != nil {
+		// TODO: understand if passing original is safe here


We should resolve this TODO or at least make it more specific before merging.

Yes, I am working on it.

alexandercampbell · 2017-05-24T20:57:25Z

pkg/kubectl/cmd/get.go

+	}
+	parts := strings.SplitN(columnStr, "=", 2)
+	if len(parts) < 2 {
+		return nil


This should be returning (opts *OutputOptions, ok bool) in accordance with Golang style guides.

Done. Thanks for the suggestion.

alexandercampbell · 2017-05-24T21:02:32Z

pkg/printers/interface.go

+
+// OutputOptions represents resource output options which is used to generate a resource printer.
+type OutputOptions struct {
+	FmtType string // e.g. json, yaml, template, jsonpath etc.


Maybe this comment should point where I can find the actual list of possible values.

I agree. Unfortunately, these are not defined as constant types in the code. For now, I have mentioned the filename where the supported format types are listed inside a function. I would like to make a separate change for defining these valid format types as constants.

mengqiy · 2017-05-24T22:29:46Z

pkg/kubectl/cmd/util/printing.go

-	allowMissingTemplateKeys := false
-	if cmd.Flags().Lookup("allow-missing-template-keys") != nil {
-		allowMissingTemplateKeys = GetFlagBool(cmd, "allow-missing-template-keys")
-	}
 	printer, generic, err := printers.GetStandardPrinter(


I think we can simplify the signature of GetStandardPrinter() by passing in outputOptions instead of outputFormat, templateFile,allowMissingTemplateKeys.

Done. Thanks for the suggestion.

mengqiy · 2017-05-24T22:48:42Z

pkg/kubectl/cmd/get.go

-		if err := printer.PrintObj(decodedObj, w); err != nil {
+		// TODO: understand if passing original is safe here
+		// if err := printer.PrintObj(decodedObj, w); err != nil {
+		if err := printer.PrintObj(original, w); err != nil {


I think we should use decodedObj, original may be an unversioned object.
Is there any issue when using decodedObj?

I ran in to a few cases where using an output format like custom-column with spec "NAME:.metadata.name,RSRC:.metadata.resourceVersion" fails with errors saying "metadata" is missing in object when passed decoded object. It works fine with original object though.
I have to root cause it but I am getting lost in the decoding function. There is some magic which I do not quite understand at this point.

droot

Addressed comments and also updated the PR. PTAL.

droot · 2017-05-24T23:39:31Z

pkg/kubectl/cmd/get.go

@@ -139,6 +139,8 @@ func NewCmdGet(f cmdutil.Factory, out io.Writer, errOut io.Writer) *cobra.Comman
 // RunGet implements the generic Get command
 // TODO: convert all direct flag accessors to a struct and pass that instead of cmd
 func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args []string, options *GetOptions) error {
+	go f.OpenAPISchema(cmdutil.GetOpenAPICacheDir(cmd))


This is to trigger eager-loading of OpenAPI schema in cache. This is running asynchronously because we don't want to block the processing of Get command because OpenAPI cache may or may for the Get command processing.

droot · 2017-05-24T23:43:09Z

pkg/kubectl/cmd/get.go

@@ -501,7 +513,9 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 			}
 			continue
 		}
-		if err := printer.PrintObj(decodedObj, w); err != nil {
+		// TODO: understand if passing original is safe here


Yes, I am working on it.

droot · 2017-05-24T23:45:26Z

pkg/kubectl/cmd/get.go

-		if err := printer.PrintObj(decodedObj, w); err != nil {
+		// TODO: understand if passing original is safe here
+		// if err := printer.PrintObj(decodedObj, w); err != nil {
+		if err := printer.PrintObj(original, w); err != nil {


I ran in to a few cases where using an output format like custom-column with spec "NAME:.metadata.name,RSRC:.metadata.resourceVersion" fails with errors saying "metadata" is missing in object when passed decoded object. It works fine with original object though.
I have to root cause it but I am getting lost in the decoding function. There is some magic which I do not quite understand at this point.

droot · 2017-05-24T23:54:01Z

pkg/kubectl/cmd/get.go

+	}
+	parts := strings.SplitN(columnStr, "=", 2)
+	if len(parts) < 2 {
+		return nil


Done. Thanks for the suggestion.

droot · 2017-05-25T22:40:04Z

pkg/kubectl/cmd/util/printing.go

-	allowMissingTemplateKeys := false
-	if cmd.Flags().Lookup("allow-missing-template-keys") != nil {
-		allowMissingTemplateKeys = GetFlagBool(cmd, "allow-missing-template-keys")
-	}
 	printer, generic, err := printers.GetStandardPrinter(


Done. Thanks for the suggestion.

droot · 2017-05-25T22:48:39Z

pkg/printers/interface.go

+
+// OutputOptions represents resource output options which is used to generate a resource printer.
+type OutputOptions struct {
+	FmtType string // e.g. json, yaml, template, jsonpath etc.


I agree. Unfortunately, these are not defined as constant types in the code. For now, I have mentioned the filename where the supported format types are listed inside a function. I would like to make a separate change for defining these valid format types as constants.

droot

Addressed comments and updated the PR with additional comments. @pwittrock @mengqiy PTAL.

smarterclayton · 2017-06-06T14:36:43Z

Um, given how this overlaps with server side get, I'm not sure I approve this.

smarterclayton · 2017-06-06T14:37:16Z

I do not want to add this to the client in 1.7. I want this to be on the server side. If we add this to the client, we'll just remove it in 1.8. What's the point of that?

smarterclayton · 2017-06-06T14:37:46Z

How did this get into the queue without the discussion resolution from the original proposal, saying that this is moving to the server.

smarterclayton · 2017-06-06T14:57:53Z

My primary concern is that this is moving to the server in 1.8, so this flag would be removed and no one would every specify it. So given that, if the desire to get something in for service catalog is very high, this needs to be an --experimental-* flag, and we need to say that the flag will be removed in 1.8.

droot · 2017-06-06T17:43:39Z

@smarterclayton I have updated the PR and renamed the flag to '--experimental-use-openapi-print-columns'. Can you pl. take a look and remove "do-not-merge" label. Lets re-evaluate if we want to deprecate or remove this functionality from kubectl when we move this logic to server side in 1.8.

pwittrock · 2017-06-06T17:50:34Z

How did this get into the queue without the discussion resolution from the original proposal, saying that this is moving to the server.

@smarterclayton Please refer to the email thread "get consensus on displaying resources - sig-apimachinery / sig-cli" (sent April 17). There you requested that we get together quickly over VC and resolve the discussion since it had been languishing. The consensus was that while there was some overlap in the 2 solutions, they were not mutually exclusive.

One of the artifacts from that discussion was - should clients be able to print an object they have without making a request to the server. e.g. does a Java client that wants to print an object it already fetch need to make a second request to the server to display the object? Also how can clients display object configuration for resources defined locally or deleted from the server? We agreed that it would be useful for clients to have some notion for how to print resources without a server roundtrip.

pwittrock · 2017-06-06T18:22:21Z

Making the flag experimental is good idea and we should do that. I would also make it deprecated so it is hidden.

smarterclayton · 2017-06-06T20:19:39Z

Ok, I vaguely recall us saying:

this would be best effort (which means the openapi spec doesn't fit under our normal rules of backwards compatibility)
we'd fail gracefully if we can't fetch the columns

I had assumed that when the rest of the server side get plumbing went in, there would be no need for the flag when offline (since we would simply determine that the object was unrecognized, make a best effort pass to get the open api spec, and then print).

The rename to "experimental" satisfies my concerns about the public API shape, objection withdrawn with that change. For 1.8, I'd expect for us to remove the flag and do the best effort fetch alongside the server side fetch attempt.

Get command now uses metadata x-kubernetes-print-columns, if present, in Openapi schema to format output for a resource. This functionality is guarded by a boolean flag 'use-openapi-print-columns'.

droot · 2017-06-06T20:43:18Z

Thanks @smarterclayton
@pwittrock marked the flag deprecated as well. PTAL. I need LGTM to get this PR merged now.

droot · 2017-06-06T21:28:13Z

@k8s-bot pull-kubernetes-bazel test this

pwittrock · 2017-06-06T21:54:29Z

@smarterclayton SGTM. Yes, this will be a best effort fetch and very small and fast in 1.8 (when using the zipped binary proto + cache). Thanks.

pwittrock · 2017-06-06T22:39:26Z

/lgtm

k8s-github-robot · 2017-06-06T22:43:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droot, fabianofranz, pwittrock, wojtek-t

Associated issue: 22

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/kubectl/OWNERS~~ [fabianofranz,pwittrock,wojtek-t]
~~pkg/printers/OWNERS~~ [fabianofranz,pwittrock,wojtek-t]

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

k8s-github-robot · 2017-06-07T07:23:40Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-06-07T07:27:45Z

Automatic merge from submit-queue (batch tested with PRs 46235, 44786, 46833, 46756, 46669)

k8s-ci-robot · 2017-06-07T08:22:41Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	f59769463bcd02d43214382d8f5b5d23b13659f0	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`
pull-kubernetes-e2e-gce-etcd3	`f768a63`	link	`@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this`

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.

smarterclayton · 2017-11-15T20:06:13Z

Why did we introduce OutputOptions instead of adding those fields to PrintOptions? I don't see anything special about them that justifies a separate object.

If there's no reason to have it separate, I'm going to move it into print options.

droot · 2017-11-15T21:56:34Z

@smarterclayton I think the main distinction is that OutputOptions is used to determine a printer type to use while PrintOptions are simply args to a given Printer to format the output.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 22, 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 May 22, 2017

k8s-github-robot assigned janetkuo and soltysh May 22, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 22, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 22, 2017

pwittrock reviewed May 22, 2017

View reviewed changes

droot force-pushed the cmd-printer-refactor branch from 76904f0 to 3ce6b91 Compare May 24, 2017 20:45

k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 24, 2017

droot commented May 24, 2017

View reviewed changes

alexandercampbell reviewed May 24, 2017

View reviewed changes

mengqiy reviewed May 24, 2017

View reviewed changes

droot force-pushed the cmd-printer-refactor branch from 3ce6b91 to b84cccc Compare May 25, 2017 22:51

droot commented May 25, 2017

View reviewed changes

droot force-pushed the cmd-printer-refactor branch 4 times, most recently from 7c13dce to 0e83d2a Compare May 31, 2017 00:19

droot commented May 31, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2017

smarterclayton removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 6, 2017

smarterclayton added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 6, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2017

droot force-pushed the cmd-printer-refactor branch from f597694 to 3930410 Compare June 6, 2017 17:38

smarterclayton removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 6, 2017

Get cmd uses print-column extn from Openapi schema

f768a63

Get command now uses metadata x-kubernetes-print-columns, if present, in Openapi schema to format output for a resource. This functionality is guarded by a boolean flag 'use-openapi-print-columns'.

droot force-pushed the cmd-printer-refactor branch from 3930410 to f768a63 Compare June 6, 2017 20:37

k8s-ci-robot assigned pwittrock Jun 6, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2017

k8s-github-robot merged commit a42867f into kubernetes:master Jun 7, 2017

Get command uses print-column extn from Openapi schema #46235

Get command uses print-column extn from Openapi schema #46235

Conversation

droot commented May 22, 2017 • edited Loading

k8s-ci-robot commented May 22, 2017

k8s-ci-robot commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented May 22, 2017

pwittrock commented May 22, 2017

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

smarterclayton commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

droot commented Jun 6, 2017

pwittrock commented Jun 6, 2017

pwittrock commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

droot commented Jun 6, 2017

droot commented Jun 6, 2017

pwittrock commented Jun 6, 2017

pwittrock commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

k8s-github-robot commented Jun 7, 2017

k8s-github-robot commented Jun 7, 2017

k8s-ci-robot commented Jun 7, 2017

smarterclayton commented Nov 15, 2017

droot commented Nov 15, 2017

droot commented May 22, 2017 •

edited

Loading