Custom additionalPrinterColumns for created CRDs #478
Conversation
Signed-off-by: petar-cvit <[email protected]>
Signed-off-by: petar-cvit <[email protected]>
Signed-off-by: petar-cvit <[email protected]>
There was a problem hiding this comment.
Thank you for your PR @petar-cvit ! The PR implementation for custom additionalPrinterColumns looks correct overall.
However i wanted to explore an alternative approach with you, what if we enhanced the SimpleSchema instead of introducing a new top level field? users could potentially do something like:
schema:
spec:
name: string | required=true print=true # or kubectl_print=trueI recognize this wouldn't be sufficient for complex printer columns with additional attributes.
I also just came across KEP 4602 (https://github.com/kubernetes/enhancements/pull/4602/files) which seems relevant. We might want to explore this direction in parallel as it could influence our approach here.
Would love to hear your thoughts here :)
|
Hey @a-hilaly, thanks for the alternative approach! If we want to add the printer columns to the simple schema, we can easily get the JSONPath for a specific field. Also, this seems easier for users to set up. I was also thinking about whether In the case of CEL columns (as described in the KEP), tieing a CEL expression to a field using simple schema doesn't seem like the right fit, primarily because CEL expressions work with the whole CRs and can get data from multiple fields at once (KEP example My point is that if we want to design it with CEL columns in mind, simple schema is probably not the way to go, but not sure how much should a KEP influence design decisions. One argument for having a dedicated Let me know what you think about the points I mentioned. Happy to discuss further |
|
Yeah, I totally agree with everything you said @petar-cvit . Keeping |
|
@petar-cvit BTW there's an ongoing thread in our Slack about introducing simple schema custom types and whether they should live directly in |
| func newCRDAdditionalPrinterColumns(additionalPrinterColumns []extv1.CustomResourceColumnDefinition) []extv1.CustomResourceColumnDefinition { | ||
| if len(additionalPrinterColumns) == 0 { | ||
| return defaultAdditionalPrinterColumns | ||
| } | ||
|
|
||
| return additionalPrinterColumns | ||
| } |
There was a problem hiding this comment.
Thought: what are your thoughts on injecting specific default CustomResourceColumnDefinition objects, when they aren't specfied by users? that way we ensure that STATE/SYNCED/AGE are always included?
There was a problem hiding this comment.
My reasoning was that users might not need the default columns and might want to remove them. If they are already customizing printer columns, my guess is that they want to show a specific state of their CRs.
However, I don't have such a strong opinion on this, and I can merge the defined additionalPrinterColumns with the default ones based on the name. Let me know if you agree with the point above or if we should add the default columns, and I can add them.
There was a problem hiding this comment.
I think allowing users to override our default printer columns seems reasonable. If they want to maintain the existing ones, they should be able to.
I mentioned above we should call that out in the docs though
|
Thanks for the link to the discussion @a-hilaly. I left a comment with a different approach in the thread that might allow you to reuse types in multiple RGDs. Let me know if it makes sense or if it's overkill for this problem |
| @@ -43,6 +44,11 @@ type ResourceGraphDefinitionSpec struct { | |||
| // | |||
| // +kubebuilder:validation:Optional | |||
| DefaultServiceAccounts map[string]string `json:"defaultServiceAccounts,omitempty"` | |||
| // AdditionalPrinterColumns defines additional printer columns that will | |||
There was a problem hiding this comment.
Worth calling out that setting this overrides the default printer columns, and if the user wants to retain the default columns they will need to specify them here explicitly?
| func newCRDAdditionalPrinterColumns(additionalPrinterColumns []extv1.CustomResourceColumnDefinition) []extv1.CustomResourceColumnDefinition { | ||
| if len(additionalPrinterColumns) == 0 { | ||
| return defaultAdditionalPrinterColumns | ||
| } | ||
|
|
||
| return additionalPrinterColumns | ||
| } |
There was a problem hiding this comment.
I think allowing users to override our default printer columns seems reasonable. If they want to maintain the existing ones, they should be able to.
I mentioned above we should call that out in the docs though
closes #476
Description
Adds
additionalPrinterColumnsto RGD and maps the columns to the created CRD in the RGD reconciler.cc: @a-hilaly