Skip to content

Conversation

jgillich
Copy link
Contributor

In Go, when a struct field is not set, it becomes a struct with
default values for all fields. These default values are included
during serialization. This causes issues with schema validation
where optional fields cannot be omitted because default values
are considered invalid.

This patch addresses this issue for Resources fields on several
types by using a pointer value.

@FxKu
Copy link
Member

FxKu commented Aug 23, 2021

LGTM @jgillich
Maybe we can use this PR to go one step further and remove the required flag from resources in the CRD schema validation.
Here, here and here. Plus, remove for connection pooler as well. You or me can also do it in a separate PR.

@FxKu FxKu added this to the 1.7 milestone Aug 23, 2021
@FxKu
Copy link
Member

FxKu commented Aug 23, 2021

Speaking of the pooler. Can you do the same change on the pooler struct too @jgillich?

In Go, when a struct field is not set, it becomes a struct with
default values for all fields. These default values are included
during serialization. This causes issues with schema validation
where optional fields cannot be omitted because default values
are considered invalid.

This patch addresses this issue for `Resources` fields on several
types by using a pointer value.
@jgillich
Copy link
Contributor Author

Maybe we can use this PR to go one step further and remove the required flag from resources in the CRD schema validation.
Here, here and here. Plus, remove for connection pooler as well. You or me can also do it in a separate PR.

If this is something we want to do then we'll also have to turn all fields on Resources and ResourceDescription into pointer values or they won't be optional when constructed in Go code. I don't know the code base very well and it's easy to miss a nil check in some code paths so I don't know if I'm comfortable doing this. Unless you can ensure me the struct is always passed to generateResourceRequirements before being accessed. 😅

Speaking of the pooler. Can you do the same change on the pooler struct too @jgillich?

I believe already did - but I accidentally removed the composition, that has been fixed.

@FxKu
Copy link
Member

FxKu commented Aug 26, 2021

Thanks @jgillich for the update. Yeah, lets lift the requirement another time.

@FxKu FxKu modified the milestones: 1.7, 1.8 Aug 27, 2021
@jopadi
Copy link
Member

jopadi commented Mar 18, 2022

👍

1 similar comment
@FxKu
Copy link
Member

FxKu commented Mar 18, 2022

👍

@FxKu FxKu merged commit f3b83c0 into zalando:master Mar 18, 2022
@FxKu
Copy link
Member

FxKu commented Mar 18, 2022

Thanks @jgillich for your contribution. Sorry it took so long to get it merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants