🌱 Enable optionalorrequired linter and add required/optional markers#3183
🌱 Enable optionalorrequired linter and add required/optional markers#3183lentzi90 wants to merge 1 commit into
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[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 |
k8s-ci-robot
added
size/L
| Name string `json:"name"` | ||
|
|
||
| // description is a human-readable description for the port. | ||
| // +required |
There was a problem hiding this comment.
This is perhaps the most controversial thing in the PR. We don't have omitempty, so I am making it required. If the field was set by humans I would do it the other way, but this is always set by the controller so I think it makes more sense this way.
There was a problem hiding this comment.
The concern I have with required here is that it's a schema-level constraint, but "the controller always sets this" is a runtime guarantee. Those two things operate at different levels. Specifically:
- Required fields can cause validation errors between object creation and the first reconcile
- If the controller ever fails mid-reconcile and writes a partial status, required fields could cause issues
- If at the future for whatever reason we need to change the controller behavior, we will have to go through an api change.
For this specific change, I don't think we are hit by 1 + 2 as this is a sub-struct under status that is not required and I guess all those fields are set by OpenStack once a port is created and therefore the controller can fully populate them, but i do think there might be some edge-cases that we are hit by this. ( Also macAddress below is already set to optional, I'm wondering why. )
I was looking a bit into k8s/capi best-practises/conventions that go over similar stuff. I found this once populated on capi's conventions.
In general I'm not fully confident i can evaluate this and the impact it might have. It feels to me that opting for required is a bit "strict" and could cause problems down the road, but I dont have the experience/knowledge to really substantiate it.
There was a problem hiding this comment.
Thanks for the input @nikParasyr ! I really appreciate it and I think you pointed out two good reasons to switch this to optional.
- CAPI guidance that status fields MUST be optional
- the MAC address below
With this in mind I think it was a mistake that this was missing omitempty. We should simply make it optional.
There was a problem hiding this comment.
Now changed to optional. The omitempty will be required/added when we enable the optionalfields linter.
|
/cc @nikParasyr |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
nikParasyr
left a comment
nikParasyr
left a comment
There was a problem hiding this comment.
A bit hesitant to lgtm this one. made a comment with my concerns
| Name string `json:"name"` | ||
|
|
||
| // description is a human-readable description for the port. | ||
| // +required |
There was a problem hiding this comment.
The concern I have with required here is that it's a schema-level constraint, but "the controller always sets this" is a runtime guarantee. Those two things operate at different levels. Specifically:
- Required fields can cause validation errors between object creation and the first reconcile
- If the controller ever fails mid-reconcile and writes a partial status, required fields could cause issues
- If at the future for whatever reason we need to change the controller behavior, we will have to go through an api change.
For this specific change, I don't think we are hit by 1 + 2 as this is a sub-struct under status that is not required and I guess all those fields are set by OpenStack once a port is created and therefore the controller can fully populate them, but i do think there might be some edge-cases that we are hit by this. ( Also macAddress below is already set to optional, I'm wondering why. )
I was looking a bit into k8s/capi best-practises/conventions that go over similar stuff. I found this once populated on capi's conventions.
In general I'm not fully confident i can evaluate this and the impact it might have. It feels to me that opting for required is a bit "strict" and could cause problems down the road, but I dont have the experience/knowledge to really substantiate it.
This commit enables the `optionalorrequired` linter in `.golangci-kal.yml` and updates all API type definitions in v1beta2 to use the standardized `+required` and `+optional` markers instead of `+kubebuilder:validation:Required`. Signed-off-by: Lennart Jern <lennart.jern@est.tech>
lentzi90
force-pushed
the
lentzi90/kal-optionalrequired
branch
from
83e4142 to
a71c03c
Compare
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
the
lgtm
4 participants
What this PR does / why we need it:
This commit enables the
optionalorrequiredlinter in.golangci-kal.ymland updates all API type definitions in v1beta2 to use the standardized+requiredand+optionalmarkers instead of+kubebuilder:validation:Required.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
/hold