apiextensions-apiserver: add columns to CRD spec#60991
apiextensions-apiserver: add columns to CRD spec#60991k8s-github-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
/cc @kubernetes/sig-api-machinery-api-reviews |
There was a problem hiding this comment.
this does not even belong under validation.
|
/cc @yliaog |
|
/assign @soltysh @juanvallejo can you comment on the API? |
There was a problem hiding this comment.
Do we want to support Type string with OpenAPI types? Currently every cell is returned as string.
There was a problem hiding this comment.
I'd start with strings, for now. We can always improve if needed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this 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.
There was a problem hiding this comment.
I'd start with strings, for now. We can always improve if needed.
42365ba to
accbd80
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should we default the field to the default columns?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
if we enforce name and createAt, I suggest to rename this field to AdditionalColumns and don't default it.
There was a problem hiding this comment.
Or even AdditionalPrinterColumns.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
CustomResourceColumnDefinition
There was a problem hiding this comment.
To match TableColumnDefinition in meta/v1beta1
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have restricted the types for CRDs via validation to the primitive types for now: string, bool, integer, number.
|
/retest Review the full test history for this PR. Silence the bot with an |
14 similar comments
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
New changes are detected. LGTM label has been removed. |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
@sttts: The following test failed, say
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. DetailsInstructions 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. |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
Follow-up of #60269.