This repository has been archived by the owner on May 6, 2022. It is now read-only.

Add custom columns to OpenAPI schema #1597

Merged

kibbles-n-bytes merged 3 commits into kubernetes-retired:master from luksa:kubectl_custom_columns

Feb 6, 2018

Contributor

luksa commented Nov 22, 2017 •

edited

Loading

Fixes #1361

This adds the required x-kubernetes-print-columns extensions to the OpenAPI schema, which allow kubectl to show sensible columns when listing service catalog resources:

$ kubectl get clusterservicebrokers
NAME         STATUS
ups-broker   FetchedCatalog

$ kubectl get clusterserviceclasses
NAME                                   EXTERNAL-NAME           BROKER       BINDABLE
4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468   user-provided-service   ups-broker   true

$ kubectl get clusterserviceplans
NAME                                   EXTERNAL-NAME   BROKER       CLASS
86064792-7ea2-467b-af93-ac9694d96d52   default         ups-broker   4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468
cc0d7529-18e8-416d-8946-6f7456acd589   premium   ups-broker   4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468

$ kubectl get serviceinstances
NAME           CLASS                   PLAN      STATUS
ups-instance   user-provided-service   default   ProvisionedSuccessfully

$ kubectl get servicebindings
NAME          INSTANCE       SECRET        STATUS
ups-binding   ups-instance   ups-binding   InjectedBindResult

I've also added the AGE column (like the one displayed for core k8s resources), but that requires changes to kubectl, so I'll put that into a different PR.

Note:

this PR also Includes changes from PR make apiserver serve openapi #1208
to try it out, you need the to build kubectl from master, or use the --experimental-use-openapi-print-columns option

k8s-ci-robot added cncf-cla: yes size/L labels

This was referenced Nov 23, 2017

Bad column alignment when printing custom columns from OpenAPI schema kubernetes/kubernetes#56282

Closed

Show AGE column for service catalog resources in kubectl #1598

Closed

luksa force-pushed the kubectl_custom_columns branch from d888b71 to 5954926 Compare

December 20, 2017 08:08

k8s-ci-robot added size/M and removed size/L labels

luksa force-pushed the kubectl_custom_columns branch from 5954926 to 00e11fe Compare

December 20, 2017 11:49

pmorie reviewed

View reviewed changes

Contributor

pmorie left a comment

I like the direction this moves us in a lot, thanks for doing this.

I do have some concerns about how status is handled (suggest we punt status to another PR), and a couple suggestions for columns to add.

pkg/apis/servicecatalog/v1beta1/types.go Outdated

@@ @@ -427,6 +430,7 @@ type ExtraValue []string @@
               // In the future, this will be allowed and will represent the intention that
               // the ServiceInstance should have the plan and/or parameters updated at the
               // ClusterServiceBroker.
+              // +k8s:openapi-gen=x-kubernetes-print-columns:custom-columns=NAME:.metadata.name,CLASS:.spec.clusterServiceClassExternalName,PLAN:.spec.clusterServicePlanExternalName,STATUS:.status.conditions[*].reason

Contributor

pmorie Jan 4, 2018

I wonder how flexible we can be here with status. I'm not sure the current formulation of status is going to be useful - how about remove the STATUS bits and deal with status in a subsequent PR?

pkg/apis/servicecatalog/v1beta1/types.go

@@ @@ -320,6 +322,7 @@ type ClusterServicePlanList struct { @@
               // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
               // ClusterServicePlan represents a tier of a ServiceClass.
+              // +k8s:openapi-gen=x-kubernetes-print-columns:custom-columns=NAME:.metadata.name,EXTERNAL NAME:.spec.externalName,BROKER:.spec.clusterServiceBrokerName,CLASS:.spec.clusterServiceClassRef.name

Contributor

pmorie Jan 4, 2018

I think the CLASS column as it is defined here is probably useful and an improvement over nothing.

With that said I think this is an area where a kubectl plugin could help immensely.

pkg/apis/servicecatalog/v1beta1/types.go Outdated

@@ @@ -225,6 +226,7 @@ type ClusterServiceClassList struct { @@
               // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
               // ClusterServiceClass represents an offering in the service catalog.
+              // +k8s:openapi-gen=x-kubernetes-print-columns:custom-columns=NAME:.metadata.name,EXTERNAL NAME:.spec.externalName,BROKER:.spec.clusterServiceBrokerName,BINDABLE:.spec.bindable

Contributor

pmorie Jan 4, 2018

The columns proposed here are good - how able adding plan_updateable?

pkg/apis/servicecatalog/v1beta1/types.go Outdated

@@ @@ -27,6 +27,7 @@ import ( @@
               // ClusterServiceBroker represents an entity that provides
               // ClusterServiceClasses for use in the service catalog.
+              // +k8s:openapi-gen=x-kubernetes-print-columns:custom-columns=NAME:.metadata.name,URL:.spec.url,STATUS:.status.conditions[*].reason

Contributor

pmorie Jan 4, 2018

Not sure on the formulation of STATUS here, how about remove it and we'll do it in another PR?

pkg/apis/servicecatalog/v1beta1/types.go Outdated

@@ @@ -691,6 +695,7 @@ type ServiceBindingList struct { @@
               // ServiceBinding represents a "used by" relationship between an application and an
               // ServiceInstance.
+              // +k8s:openapi-gen=x-kubernetes-print-columns:custom-columns=NAME:.metadata.name,INSTANCE:.spec.instanceRef.name,SECRET:.spec.secretName,STATUS:.status.conditions[*].reason

Contributor

pmorie Jan 4, 2018

Same concerns about STATUS here.

luksa added 3 commits

January 4, 2018 14:20


          Add extension x-kubernetes-print-columns to OpenAPI schema

a55a432

This makes kubectl show the proper columns for ServiceCatalog resources.


          Remove STATUS column from custom columns in OpenAPI schema

1f6c7dc


          Add PLAN UPDATABLE column to ClusterServiceClass OpenAPI custom columns

6b729bb

luksa force-pushed the kubectl_custom_columns branch from 9dfe7d9 to 6b729bb Compare

January 4, 2018 13:20

Contributor Author

luksa commented Jan 4, 2018

@pmorie Addressed all your comments & rebased to current master

Contributor Author

luksa commented Jan 4, 2018

/retest

arschles added do-not-merge and removed do-not-merge labels

Contributor

arschles commented Jan 5, 2018

@luksa @pmorie in your opinions, should we wait to merge this until an official kubectl version is out that supports custom columns?

iirc this wouldn't break anything right now so I don't see any problem in merging now

Contributor Author

luksa commented Jan 6, 2018

This can be merged immediately, as it has no effect until you enable the serving of the OpenAPI schema in the svc cat API server through the --serve-openapi-spec flag (see #1612).

Contributor

n3wscott commented Jan 13, 2018

LGTM

Contributor

kibbles-n-bytes commented Jan 22, 2018

I'm happy with this going in.

kibbles-n-bytes added the LGTM1 label

MHBauer approved these changes

View reviewed changes

Contributor

MHBauer left a comment

LGTM

I don't think this could break anything.

MHBauer added the LGTM2 label

kibbles-n-bytes merged commit 6d809c3 into kubernetes-retired:master

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cncf-cla: yes LGTM1 LGTM2 size/M