Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apiextensions-apiserver: add columns to CRD spec #60991

Merged
merged 2 commits into from May 28, 2018

Conversation

@sttts
Copy link
Contributor

sttts commented Mar 9, 2018

Follow-up of #60269.

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

This comment has been minimized.

Copy link
Contributor Author

sttts commented Mar 9, 2018

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

// Columns

This comment has been minimized.

@sttts

sttts Mar 9, 2018

Author Contributor

this does not even belong under validation.

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

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

This comment has been minimized.

@lcfang

lcfang Mar 12, 2018

Contributor

can we delete this "TODO"?

This comment has been minimized.

@sttts

sttts Mar 12, 2018

Author Contributor

Yes, we should.

This comment has been minimized.

@sttts

sttts Apr 4, 2018

Author Contributor

done

@jennybuckley

This comment has been minimized.

Copy link
Contributor

jennybuckley commented Mar 15, 2018

/cc @yliaog

@k8s-ci-robot k8s-ci-robot requested a review from yliaog Mar 15, 2018

@juanvallejo juanvallejo referenced this pull request Mar 16, 2018

Open

Server-side printing continued #60712

5 of 13 tasks complete
@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Apr 3, 2018

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

// 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

This comment has been minimized.

@sttts

sttts Apr 4, 2018

Author Contributor

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

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

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

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2018

Contributor

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.

This comment has been minimized.

@sttts

sttts Apr 27, 2018

Author Contributor

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?

@sttts sttts force-pushed the sttts:sttts-crd-columns branch from f1d30b5 to c9c7702 Apr 4, 2018

@sttts sttts force-pushed the sttts:sttts-crd-columns branch from c9c7702 to a4ba14a Apr 6, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

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.

// 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

This comment has been minimized.

@soltysh

soltysh Apr 10, 2018

Contributor

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

@sttts sttts force-pushed the sttts:sttts-crd-columns branch from 42365ba to accbd80 Apr 12, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

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

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 {

This comment has been minimized.

@soltysh

soltysh Apr 12, 2018

Contributor

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.

This comment has been minimized.

@sttts

sttts Apr 17, 2018

Author Contributor

Should we default the field to the default columns?

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2018

Contributor

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.

This comment has been minimized.

@sttts

sttts May 11, 2018

Author Contributor

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

This comment has been minimized.

@sttts

sttts May 11, 2018

Author Contributor

Or even AdditionalPrinterColumns.

This comment has been minimized.

@soltysh

soltysh May 21, 2018

Contributor

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.

Columns []Column
}

type Column struct {

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2018

Contributor

CustomResourceColumnDefinition

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2018

Contributor

To match TableColumnDefinition in meta/v1beta1

This comment has been minimized.

@sttts

sttts May 11, 2018

Author Contributor

done

// 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

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2018

Contributor

Call this JSONPath

This comment has been minimized.

@sttts

sttts May 11, 2018

Author Contributor

done

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

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2018

Contributor

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.

This comment has been minimized.

@sttts

sttts May 18, 2018

Author Contributor

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 26, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

13 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 26, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 27, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 28, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 28, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 28, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

sttts added some commits Mar 9, 2018

@sttts sttts force-pushed the sttts:sttts-crd-columns branch from f6482ec to 96475ce May 28, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 28, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 28, 2018

@sttts sttts added the lgtm label May 28, 2018

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 28, 2018

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 28, 2018

@sttts: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-device-plugin-gpu 96475ce link /test pull-kubernetes-e2e-gce-device-plugin-gpu

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.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 28, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 34383aa into kubernetes:master May 28, 2018

14 of 18 checks passed

pull-kubernetes-e2e-gce-device-plugin-gpu Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation sttts authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.