🐛 Add support for empty maps or lists #863

Danil-Grigorev · 2023-11-23T17:23:40Z

Fixes: #550

This PR adds support for kubebuilder:default={} type of annotations in controller tools.

JoelSpeed · 2023-11-23T17:40:00Z

What is the motivation here, it's unclear to me why you'd want to default to an empty struct?
What happens if there are defaults defined within the struct that is being defaulted to empty?
What happens if there are required fields in the struct that is being defaulted to empty?

Danil-Grigorev · 2023-11-24T10:56:28Z

The motivation here is to fix a small bug here.

Empty default for a map/struct is a subset of a default with some specified values. {a: b} should work the same way as {}.
This would result in an empty field present in the resource. Like status: {}. Useful when a serialized version of the resource is sent as an input (data) for some other request.
Any type of default here is contradictory I think.

This is more wasteful to the storage space, but if this is needed, controller-gen would support this.

sttts · 2023-11-24T12:14:23Z

You must default to an empty object to activate the defaults deep in the object. This is intentionally the way this works in CRDs. Compare https://github.com/kube-bind/kube-bind/blob/main/deploy/patches/kube-bind.io_apiserviceexportrequests.yaml-patch.

JoelSpeed · 2023-11-24T12:44:42Z

You must default to an empty object to activate the defaults deep in the object. This is intentionally the way this works in CRDs. Compare https://github.com/kube-bind/kube-bind/blob/main/deploy/patches/kube-bind.io_apiserviceexportrequests.yaml-patch.

This is kinda what I was looking for, some proof that defaulting to the empty object would allow recursive defaults to work. I wasn't sure if that's how the API server worked or not. As we have an example of that, I have no objection to fixing this bug.

JoelSpeed · 2023-11-24T12:47:12Z

I think we should update the cronjob types and the tests that rely on them to cover this case at the integration layer as well

Danil-Grigorev · 2023-11-24T13:33:25Z

Added a test case in the cronjobs testdata, hope this look good.

JoelSpeed · 2023-11-24T14:06:08Z

pkg/crd/testdata/cronjob_types.go

@@ -297,6 +309,11 @@ type MinMaxObject struct {
 	Baz string `json:"baz,omitempty"`
 }

+type EmpiableObject struct {
+	Foo string `json:"foo,omitempty"`


Can you add a default to the field so that we can prove that the empty default on the parent triggers the inner field to be defaulted.

So with a modified version of CRD, where only these 2 new fields are present, this is how the applied CR looks like:

kind: CronJob apiVersion: testdata.kubebuilder.io/v1 metadata: name: test namespace: default spec: {}

results in

apiVersion: testdata.kubebuilder.io/v1 kind: CronJob metadata: annotations: kubectl.kubernetes.io/last-applied-configuration: | {"apiVersion":"testdata.kubebuilder.io/v1","kind":"CronJob","metadata":{"annotations":{},"name":"test","namespace":"default"},"spec":{}} creationTimestamp: "2023-11-24T16:21:31Z" generation: 1 name: test namespace: default resourceVersion: "23165" uid: d86f95e1-54be-4e04-b789-cb997bc3dd17 spec: defaultedEmptyObject: foo: forty-two defaultedEmptySlice: []

@JoelSpeed Do you think all the comments are addressed? This is quite useful functionality, and it would be best to avoid manually patching resource, and rely on the annotation instead.

JoelSpeed · 2023-11-24T14:06:33Z

pkg/crd/testdata/cronjob_types.go

@@ -120,6 +120,18 @@ type CronJobSpec struct {
 	// +kubebuilder:example={{nested: {foo: "baz", bar: true}},{nested: {bar: false}}}
 	DefaultedObject []RootObject `json:"defaultedObject"`

+	// This tests that empty slice defaulting can be performed.
+	// +kubebuilder:default={}


Is {} a valid default for slice?

Good question, this needs to be fixed. CRD does not complain for deafult: [] though

Everything is fixed in the current commit.

It seems like {} is the correct syntax for an empty array.

This is strange. But I guess fine given it's not JSON but our custom syntax (?)

I agree it's strange, but too late to change it now? Probably a reason for not using the obvious [] for arrays in marker syntax?

Yup seems definitely too late to change it now. Given that non-empty arrays worked already in the past :)

erikgb

Thanks @Danil-Grigorev! This looks like an important fix to me.

/lgtm

erikgb · 2023-12-05T18:43:57Z

pkg/crd/testdata/cronjob_types.go

@@ -120,6 +120,18 @@ type CronJobSpec struct {
 	// +kubebuilder:example={{nested: {foo: "baz", bar: true}},{nested: {bar: false}}}
 	DefaultedObject []RootObject `json:"defaultedObject"`

+	// This tests that empty slice defaulting can be performed.
+	// +kubebuilder:default={}


It seems like {} is the correct syntax for an empty array.

JoelSpeed · 2023-12-06T11:56:02Z

/lgtm

Thanks @Danil-Grigorev

joelanford · 2023-12-06T14:19:54Z

/approve

Thanks!

k8s-ci-robot · 2023-12-06T14:20:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev, joelanford

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [joelanford]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 23, 2023

k8s-ci-robot requested review from joelanford and vincepri November 23, 2023 17:23

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 23, 2023

Danil-Grigorev force-pushed the support-empty-maps-lists branch from 681f4d1 to 000c63f Compare November 24, 2023 13:31

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 24, 2023

JoelSpeed reviewed Nov 24, 2023

View reviewed changes

Danil-Grigorev force-pushed the support-empty-maps-lists branch from 000c63f to 5af596e Compare November 24, 2023 14:53

Add support for empty maps or lists

2fac91e

Danil-Grigorev force-pushed the support-empty-maps-lists branch from 5af596e to 2fac91e Compare November 24, 2023 19:17

Danil-Grigorev requested a review from JoelSpeed November 27, 2023 11:32

erikgb reviewed Dec 5, 2023

View reviewed changes

k8s-ci-robot assigned erikgb Dec 5, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2023

k8s-ci-robot assigned JoelSpeed Dec 6, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2023

k8s-ci-robot merged commit 72d3440 into kubernetes-sigs:master Dec 6, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Add support for empty maps or lists #863

🐛 Add support for empty maps or lists #863

Danil-Grigorev commented Nov 23, 2023

JoelSpeed commented Nov 23, 2023

Danil-Grigorev commented Nov 24, 2023 •

edited

Loading

sttts commented Nov 24, 2023

JoelSpeed commented Nov 24, 2023

JoelSpeed commented Nov 24, 2023

Danil-Grigorev commented Nov 24, 2023

JoelSpeed Nov 24, 2023

Danil-Grigorev Nov 24, 2023

Danil-Grigorev Nov 29, 2023

JoelSpeed Nov 24, 2023

Danil-Grigorev Nov 24, 2023

Danil-Grigorev Nov 27, 2023

erikgb Dec 5, 2023

sbueringer Dec 6, 2023

erikgb Dec 6, 2023

sbueringer Dec 6, 2023

erikgb left a comment

erikgb Dec 5, 2023

JoelSpeed commented Dec 6, 2023

joelanford commented Dec 6, 2023

k8s-ci-robot commented Dec 6, 2023

🐛 Add support for empty maps or lists #863

🐛 Add support for empty maps or lists #863

Conversation

Danil-Grigorev commented Nov 23, 2023

JoelSpeed commented Nov 23, 2023

Danil-Grigorev commented Nov 24, 2023 • edited Loading

sttts commented Nov 24, 2023

JoelSpeed commented Nov 24, 2023

JoelSpeed commented Nov 24, 2023

Danil-Grigorev commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Dec 6, 2023

joelanford commented Dec 6, 2023

k8s-ci-robot commented Dec 6, 2023

Danil-Grigorev commented Nov 24, 2023 •

edited

Loading