🌱 Add registry+v1 bundle config unmarshal and validation layer #2278
Conversation
Signed-off-by: Per G. da Silva <[email protected]>
…amespace Signed-off-by: Per G. da Silva <[email protected]>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
camilamacedo86
left a comment
camilamacedo86
left a comment
There was a problem hiding this comment.
We are just changing the place where the check lives now to the config API
👍 /lgtm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2278 +/- ##
==========================================
+ Coverage 70.28% 70.32% +0.03%
==========================================
Files 88 90 +2
Lines 8762 8794 +32
==========================================
+ Hits 6158 6184 +26
- Misses 2190 2196 +6
Partials 414 414
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:
|
| } | ||
|
|
||
| // collect bundle install modes | ||
| installModeSet := sets.New(rv1.CSV.Spec.InstallModes...) |
There was a problem hiding this comment.
rv1 is passed into this function only to create this set of install modes. Hence, we can pass to the function these install modes directly, e.g.
func validateConfig(config *Config, rv1 *RegistryV1, installNamespace string, installModes sets.Set[v1alpha1.InstallMode]) errornit: also, perhaps would be more readable if we implement validateConfig as receiver function on Config:
function (c *Config) ValidateFor(installNamespaces string, installModes sets.Set[v1alpha1.InstallMode]) errorThere was a problem hiding this comment.
This is the case now. But, not necessarily in the future. We want to validate based on what's in the bundle. At the moment the only pertinent thing is the install mode. I think as an API, taking in the bundle makes more sense.
There was a problem hiding this comment.
I think as an API, taking in the bundle makes more sense.
This is a private function, so API can change whenever we see a need.
There was a problem hiding this comment.
oh sorry - I misread I thought this comment was under the Unmarshal function. I'll fix that up!
There was a problem hiding this comment.
But then I wouldn't make it package public though...for the reasons I sited in the first response. I think as an API taking the bundle as a parameter is the right thing
| return nil | ||
| } | ||
|
|
||
| func hasWatchNamespaceAsConfig(bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) bool { |
There was a problem hiding this comment.
the function is used once only - thus for the readability it would be ok to put it inline in validate() function. BTW, the function name mentions watchnamespace, but it does not appear in the arguments - perhaps the function name should be rephrased to reflect what it does?
There was a problem hiding this comment.
The question the function answers is whether the bundle supports the watchNamespace configuration - this is driven off the install mode support. If I don't need it again for the "required" case I'll inline it and add the appropriate comments
There was a problem hiding this comment.
I've refactored to isWatchNamespaceConfigSupported. I prefer to have these more complex clauses aliased under a more understandable function. So, I've left it in.
Signed-off-by: Per G. da Silva <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, thetechnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 238dcf6
into
operator-framework:main
4 participants
Description
Changes
UnmarshalConfigfunction to the (registry+v1) bundle package that unmarshals a json/yaml byte slice into a registry+v1 configuration struct and subsequently validates itgetWatchNamespacefunctionFor this PR we do manual registry+v1 bundle config schema validation as this is sufficient. Once the configuration surface expands (e.g. by adding SubscriptionConfig support, or others), and/or we add Helm bundle support, we'd probably want to move to a JSONSchema-based approach.
Motivation
As things were, we'd accept bundle configuration containing
watchNamespacefor any bundle. In keeping with the mental model that each bundle would provide its own configuration schema, this shouldn't be the case. E.g. There are bundles that won't have any configuration like bundles that only support AllNamespaces mode. Therefore, we needed an additional layer of validation that checked the provided configuration against what is provided by the bundle.Reviewer Checklist