✨ Update fields in the spec to no longer be a pointer #1171
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Rashmi Gottipati <[email protected]>
Signed-off-by: Rashmi Gottipati <[email protected]>
rashmigottipati
force-pushed
the
update-fields
branch
from
7848f10 to
25c5f55
Compare
| @@ -319,7 +319,7 @@ type PreflightConfig struct { | |||
| // consequences of upgrading a CRD, such as data loss. | |||
| // | |||
| // This field is required if the spec.preflight field is specified. | |||
| CRDUpgradeSafety *CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety"` | |||
| CRDUpgradeSafety CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety,omitempty"` | |||
There was a problem hiding this comment.
This field is a required field and therefore should not have the omitempty in the JSON tag:
| CRDUpgradeSafety CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety,omitempty"` | |
| CRDUpgradeSafety CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety"` |
There was a problem hiding this comment.
I originally left it without the omitempty tag, however, unit tests are failing with the below error i.e. the field having a null value when it is empty:
Error Trace: /Users/rashmigottipati/go/src/github.com/operator-framework/operator-controller/internal/controllers/clusterextension_controller_test.go:63 Error: Received unexpected error: ClusterExtension.olm.operatorframework.io "cluster-extension-test-s58x6kfr" is invalid: [spec.preflight.crdUpgradeSafety.policy: Unsupported value: "": supported values: "Enabled", "Disabled", <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation]
Since these fields are required only if the spec.preflight field is specified, there's a possibility of it being null when the spec.preflight field is not being set and so I think it would be ok to add the omitempty tag to this field and also the Policy field but open to suggestions on how to appropriately handle this in a better way.
There was a problem hiding this comment.
Is there a way we could ensure the default value is set when the top level spec.preflight field is not explicitly set?
There was a problem hiding this comment.
if not, we may need to have some more complex CEL validations in place. We need to be careful there as well though to ensure the cost of the CEL expression is not too expensive.
There was a problem hiding this comment.
I found kubernetes-sigs/controller-tools#622 (comment) which may be what we need to do in this case.
There was a problem hiding this comment.
Keep in mind. This is required for now, only because there are no other sibling preflight check configurations. As soon as there are sibling configurations (e.g. for a permissions preflight check), then we'll need a validation like "at least one of the preflight check fields is required".
With that in mind, perhaps it is reasonable to go ahead and implement a CEL expression based validation that checks "at least one of" semantics, where the check starts out as at least one of: [crdUpgradeSafety]
There was a problem hiding this comment.
Thats a good point @joelanford . I do think it makes sense to go ahead and update it such that we have the "at least one of" approach
| @@ -340,7 +340,7 @@ type CRDUpgradeSafetyPreflightConfig struct { | |||
| // | |||
| //+kubebuilder:validation:Enum:="Enabled";"Disabled" | |||
| //+kubebuilder:default:=Enabled | |||
| Policy CRDUpgradeSafetyPolicy `json:"policy"` | |||
| Policy CRDUpgradeSafetyPolicy `json:"policy,omitempty"` | |||
There was a problem hiding this comment.
This is a required field and therefore should not have the omitempty JSON tag:
| Policy CRDUpgradeSafetyPolicy `json:"policy,omitempty"` | |
| Policy CRDUpgradeSafetyPolicy `json:"policy"` |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1171 +/- ##
==========================================
+ Coverage 75.33% 75.66% +0.32%
==========================================
Files 35 35
Lines 1934 1927 -7
==========================================
+ Hits 1457 1458 +1
+ Misses 331 324 -7
+ Partials 146 145 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-robot
added
the
needs-rebase
|
Superseded by #1223 |
Description
Fixes #1146
Reviewer Checklist