-
Notifications
You must be signed in to change notification settings - Fork 113
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
Review CRD usage of pointers vs non-pointers #397
Comments
shahulsonhal
pushed a commit
to shahulsonhal/build
that referenced
this issue
Nov 18, 2021
We were not following the pointer vs non-pointer fields for custom resource definitions having required and optional fields. Fixes shipwright-io#397 Update required custom resource fields to non-pointer Update optional custom resource fields that do not have a built-in nil value to pointers
4 tasks
shahulsonhal
pushed a commit
to shahulsonhal/build
that referenced
this issue
Dec 13, 2021
We were not following the pointer vs non-pointer fields for custom resource definitions having required and optional fields. Fixes shipwright-io#397 Update required custom resource fields to non-pointer Update optional custom resource fields that do not have a built-in nil value to pointers
shahulsonhal
pushed a commit
to shahulsonhal/build
that referenced
this issue
Jan 21, 2022
We were not following the pointer vs non-pointer fields for custom resource definitions having required and optional fields. Fixes shipwright-io#397 Update required custom resource fields to non-pointer Update optional custom resource fields that do not have a built-in nil value to pointers
shahulsonhal
pushed a commit
to shahulsonhal/build
that referenced
this issue
Jan 28, 2022
We were not following the pointer vs non-pointer fields for custom resource definitions having required and optional fields. Fixes shipwright-io#397 Update required custom resource fields to non-pointer Update optional custom resource fields that do not have a built-in nil value to pointers
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In our custom resource definitions we are having some fields that are pointers and some are not. My expectation is that:
This is done mostly like this, but through Validate nil pointer exceptions during BuildRun Reconcile #396 I noticed that this loc is doing it in a different way:
build/pkg/apis/build/v1alpha1/build_types.go
Line 28 in 87b5d4c
And looking around, there is also this loc which does it imo incorrect:
build/pkg/apis/build/v1alpha1/buildrun_types.go
Line 21 in 87b5d4c
In both cases, a required field is using a reference. My proposal: quickly go through the CRDs and change the pointer usage where it is not correctly done.
For optional strings, one can argue whether one should use a pointer to allow a nil value or assume the empty string means not set. We have both cases in our code. For example:
build/pkg/apis/build/v1alpha1/buildrun_types.go
Line 49 in 87b5d4c
build/pkg/apis/build/v1alpha1/buildrun_types.go
Line 53 in 87b5d4c
The text was updated successfully, but these errors were encountered: