apiextensions-apiserver: add columns to CRD spec #60991

sttts · 2018-03-09T17:52:47Z

Follow-up of #60269.

Add spec.additionalPrinterColumns to CRDs to define server side printing columns.

sttts · 2018-03-09T17:54:22Z

/cc @kubernetes/sig-api-machinery-api-reviews

sttts · 2018-03-09T17:55:07Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

@@ -34,6 +34,16 @@ type CustomResourceDefinitionSpec struct {
 	Validation *CustomResourceValidation
 	// Subresources describes the subresources for CustomResources
 	Subresources *CustomResourceSubresources
+
+	// Columns


this does not even belong under validation.

lcfang · 2018-03-12T13:06:02Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -425,7 +425,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource
 	}

 	// TODO: identify how to pass printer specification from the CRD


can we delete this "TODO"?

Yes, we should.

jennybuckley · 2018-03-15T20:16:38Z

/cc @yliaog

sttts · 2018-04-03T13:12:43Z

/assign @soltysh @juanvallejo can you comment on the API?

sttts · 2018-04-04T11:24:49Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

+	// Header is the header to be printed on-top of the column.
+	Header string
+	// Path is a simple JSON path, i.e. with array notation.
+	Path string


Do we want to support Type string with OpenAPI types? Currently every cell is returned as string.

I'd start with strings, for now. We can always improve if needed.

We can't actually return new cells of different types in the future unless we define it now because that would not be backwards compatible for a client that assumes strings. We need to up front force people to acknowledge non-strings if we ever want to support them.

That arguments sound more like for the client side: we have to make clients aware of non-strings from the beginning. Here, it's about CRDs, i.e. whether we want to allow to return non-strings for CRs.

One related question: if the user define a column as non-string, e.g. as integer, but the cell in the CR is not convertable to integer. What should we do with that CR?

soltysh

This lgtm, but trying this out I can't get this to work, so probably there's some more wiring needed. I can look into it tomorrow.

soltysh · 2018-04-10T15:41:12Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

+	// Header is the header to be printed on-top of the column.
+	Header string
+	// Path is a simple JSON path, i.e. with array notation.
+	Path string


I'd start with strings, for now. We can always improve if needed.

soltysh

The overall state of the PR lgtm, although I'd like to hear comments from @kubernetes/sig-api-machinery-api-reviews

soltysh · 2018-04-12T09:53:28Z

.../k8s.io/apiextensions-apiserver/pkg/registry/customresource/tableconvertor/tableconvertor.go

-		if err := path.Parse(parts[1]); err != nil {
-			return c, fmt.Errorf("unrecognized column definition in 'x-kubernetes-print-columns': %v", spec)
+
+	for _, col := range crdColumns {


I've played a bit with the PR and the only comment that I have is this. If a CRD author specifies columns we should overwrite the default ones. This will provide a better UX for CRD authors, otherwise they might end up with unwanted columns. If you look closely here how we specify the columns for the built-in types we always specify full set, there's no defaulting. I'd imagine this being the same for CRDs.

Should we default the field to the default columns?

I don't think we should allow them to overwrite name. I'm ok with forcing them to specify createdAt, and that would actually be better because that's an example of a date field we should handle correctly.

Defaulting the field may make sense, not 100% sure.

if we enforce name and createAt, I suggest to rename this field to AdditionalColumns and don't default it.

Or even AdditionalPrinterColumns.

Let's default and allow them to change it later on. I don't agree with @smarterclayton here about not having the ability to drop name field. The author should have that option, and defaulting is exactly the right place for that.

smarterclayton · 2018-04-23T14:10:00Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

+	Columns []Column
+}
+
+type Column struct {


CustomResourceColumnDefinition

To match TableColumnDefinition in meta/v1beta1

smarterclayton · 2018-04-23T14:16:37Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

+	// Header is the header to be printed on-top of the column.
+	Header string
+	// Path is a simple JSON path, i.e. with array notation.
+	Path string


Call this JSONPath

smarterclayton · 2018-04-23T14:18:45Z

.../k8s.io/apiextensions-apiserver/pkg/registry/customresource/tableconvertor/tableconvertor.go

+						cells = append(cells, cell)
+						continue
+					}
+					// TODO: types.go for Cells mentiones "simple maps", but resttest.go forbid them. Clarify!


Simple maps was intended for things like labels, but we haven't gotten there yet. I think the important outcome is that clients must hide/handle types they don't recognize, but we have to realize that not all clients will do that.

I have restricted the types for CRDs via validation to the primitive types for now: string, bool, integer, number.

fejta-bot · 2018-05-26T16:29:04Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.