🐛 (fix) Priority field to use pointer type with correct markers #2413
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| // | ||
| // +kubebuilder:default:=0 | ||
| // +kubebuilder:validation:minimum:=-2147483648 | ||
| // +kubebuilder:validation:maximum:=2147483647 |
There was a problem hiding this comment.
The above ones are invalid.
Therefore, we are fixing the validation here.
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // +kubebuilder:validation:Format:=int32 | ||
| // +optional | ||
| Priority int32 `json:"priority"` | ||
| Priority *int32 `json:"priority,omitempty"` |
There was a problem hiding this comment.
- Pointer = distinguish between "not set" (nil) vs "set to 0"
- omitempty = don't show field in YAML when not set (cleaner)
- kubebuilder default = API server fills in 0 when user omits it
There was a problem hiding this comment.
I'm not sure we can make this change since it changes the serialization of the field
There was a problem hiding this comment.
We should use a pointer.
It theoretically would be a breaking change, but because we are setting 0 by default, we do not change the behaviour
There was a problem hiding this comment.
Changing this to a pointer could break folks who've already built against this API. They are assuming that they do not have to validate today that priority is a non-nil pointer, your default is preventing this from being an issue for now, but what if we later forget that this wasn't always a pointer and decide to remove the default? Now there could be code out there that we cause NPEs in, because they were based on previous assumptions
This change seems innocuous at the moment because of the defaulting, but sets us up for bad times in the future
There was a problem hiding this comment.
Why do we need to distinguish between (1) not set and (2) set to 0? What is the use case?
We should be careful given that we introduce omitempty tag here, we can land into the following scenario:
- client creates resource with
nilpriority, but on the k8s api side, it is persisted with0, because that is the default value. - when fetching now the resource from the k8s api server, the priority is never
nil, therefore, we are not able to distinguish if it was set by the creator or not.
Also,
- client sets the priority to 2 at the creation
- later by mistake client does not emit
priorityfield at update - it is going to be reset to0and that could be confusing for users.
There was a problem hiding this comment.
when fetching now the resource from the k8s api server, the priority is never nil, therefore, we are not able to distinguish if it was set by the creator or not.
You can't currently do that either. Without omitempty and no pointer, you'll read 0 from a structured client and have no idea if that is even persisted in the API server, it may be, it may not
Currently, the API doesn't successfully round trip all values. I'm not convinced fixing this is the right thing to do though, especially given this is a v1 API.
Change ClusterCatalog Priority field from int32 to *int32 to properly represent an optional field with a default value. - Update Priority to *int32 with json:"priority,omitempty" - Fix kubebuilder markers: Minimum/Maximum capitalization - Add Format:=int32 marker for OpenAPI schema - Add GetPriority() helper method for safe nil handling - Update resolver to use GetPriority() instead of direct access - Update tests to use ptr.To[int32]() for pointer values The kubebuilder default marker ensures the API server sets Priority to 0 when omitted, while the pointer type allows proper detection of user-specified vs defaulted values. Assisted-by: CLAUDE
camilamacedo86
force-pushed
the
fix-marker-minmax
branch
from
84c7ea1 to
c88aa54
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The lowest possible value is -2147483648. | ||
| // The highest possible value is 2147483647. | ||
| // | ||
| // +kubebuilder:default:=0 |
There was a problem hiding this comment.
The +kubebuilder:default:=0 marker combined with the pointer type *int32 creates a contradiction. When this default is applied by the API server, the Go code will receive a non-nil pointer to 0 whether the user explicitly set priority to 0 or omitted it entirely. This defeats the purpose of using a pointer type, which is typically to distinguish between "not set" (nil) and "set to zero value" (non-nil pointer to 0). Consider removing the default marker if the goal is to detect user-specified vs omitted values, or changing back to a non-pointer type if a default value should always be present.
There was a problem hiding this comment.
@@JoelSpeed @everettraven Is not that accurate?
There was a problem hiding this comment.
Is 0 a valid choice for priority?
If it is, then using a pointer is correct whether there is a default or not.
If you decide you also want to default to zero, that should have no impact on the pointer/omitempty status of the field.
If you did not have a pointer, but did have omitempty (to be expected now), then you'd not be able to correctly roundtrip the 0 value here
I think copilot is wrong here
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2413 +/- ##
=======================================
Coverage 68.82% 68.82%
=======================================
Files 100 100
Lines 7641 7648 +7
=======================================
+ Hits 5259 5264 +5
- Misses 1947 1948 +1
- Partials 435 436 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/hold We always expect priority to be set or defaulted, so the field is never nil. Let's make sure we discuss the intent and implications here. I suspect we want to leave this the way it is. |
openshift-ci
bot
added
the
do-not-merge/hold
5 participants
Change ClusterCatalog Priority field from int32 to *int32 to properly
represent an optional field with a default value.
The kubebuilder default marker ensures the API server sets Priority
to 0 when omitted, while the pointer type allows proper detection of
user-specified vs defaulted values.