Uh oh!
There was an error while loading. Please reload this page.
⚠️ types conversions from review - #1223
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| // PreflightConfig holds the configuration for the preflight checks. | ||
| // PreflightConfig holds the configuration for the preflight checks. If used, at least one preflight check must be non-nil. | ||
| // +kubebuilder:validation:XValidation:rule="has(self.crdUpgradeSafety)",message="crdUpgradeSafety is required when preflight is specified" |
There was a problem hiding this comment.
The original intent of #1146 is to make the PreflightConfig optional, but if present, to require that at least one of the preflight types has been provided (this currently is a set consisting only of CRDUpgradeSafetyPreflightConfig).
In order to enforce the 'any of' check for valid preflight types, the PreflightConfig field in ClusterExtensionInstallConfig had to remain an optional/pointer.
This CEL is a result of the decision to enforce at least one valid preflight type is present IFF the PreflightConfig is present.
There was a problem hiding this comment.
I recently ran into an issue when playing around with CEL in a CRD that came back saying that I had exceeded the CEL cost budget. I haven't been able to find if this is a cost budget for the entire CRD or an individual CEL expression but realizing that there even is one made me think that we are better off using the anyOf field in the CRD to enforce the "at least one of" clause (to limit the use of CEL to only where it is absolutely necessary).
Maybe @joelanford has some thoughts on this?
There was a problem hiding this comment.
Unfortunately there is no controller-gen support for automatically creating the anyOf, oneOf, etc. directives but maybe that is something we could try to contribute in the future?
There was a problem hiding this comment.
Oh yeah, if this is an area that we need "at least one" semantics and that's what anyOf does, let's do that instead.
There was a problem hiding this comment.
Example in rukpak that may help (it uses oneOf, not anyOf, but I assume it is otherwise an example of what we want to do.
There was a problem hiding this comment.
Aside from preferential semantics, what's the difference between has(self.crdUpgradeSafety) || has(self.NewType || ... and anyOf? It feels like the only real difference is manual vs controller-gen maintenance.
There was a problem hiding this comment.
I think the biggest difference is there is no "cost budget" associated with the anyOf. That being said, I just played around with chaining has(...) OR statements in https://playcel.undistro.io/ and it doesn't look like there is a cost budget increase for the chaining and it stays statically at a cost of 1.
My original concern was that as the subset of fields grew so would our cost of evaluation, but that seems to not be the case.
With that in mind, I'm OK with CEL going in if you feel that we should use CEL here @grokspawn
There was a problem hiding this comment.
The runtime-cost-budget suggest that the intent is to avoid runaway evaluation costs.
The resource use by validation functions says specifically
You are unlikely to encounter issues with the resource budget for validation if you only specify rules that always take the same amount of time regardless of how large their input is.
I don't believe that the anyOf approach has(self.property) [|| has(self.property2)... || has(self.propertyN)] suffers from a non-linear cost (internally, this should refer to a limited sequence of nil pointer checks with early escape), so I don't think that this rises to the level of something that we'd want to maintain via kustomize patch instead (with accompanying maxItems, maxProperties, maxLength presumably).
c953398 to
a83baa3CompareCodecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #1223 +/- ##
=======================================
Coverage 76.53% 76.53% =======================================
Files 40 40 Lines 2340 2340 =======================================
Hits 1791 1791 Misses 392 392 Partials 157 157
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
perdasilva
commented
Sep 6, 2024
/lgtm |
New changes are detected. LGTM label has been removed. |
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
87dcaaf to
e43bedeCompareSigned-off-by: Jordan Keister <jordan@nimblewidget.com>
e43bede to
86d2e02Compare
Fixes#1146
Replaces #1171
Description
Reviewer Checklist