✨ OPRUN-4090: (rukpak) extend bundle renderer to accept config opts #2166
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
anik120
changed the title
anik120
force-pushed
the
registryv1-renderer-config-support
branch
2 times, most recently
from
3577b5b to
cb7ab21
Compare
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2166 +/- ##
==========================================
+ Coverage 72.79% 72.92% +0.13%
==========================================
Files 79 79
Lines 7340 7410 +70
==========================================
+ Hits 5343 5404 +61
- Misses 1651 1659 +8
- Partials 346 347 +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:
|
anik120
force-pushed
the
registryv1-renderer-config-support
branch
2 times, most recently
from
d4af9f8 to
f26e1fa
Compare
Introduce BundleConfig that contains InstallConfig and DeploymentConfig.
anik120
force-pushed
the
registryv1-renderer-config-support
branch
from
f26e1fa to
7b919f4
Compare
| case !allSupported && !singleSupported && !ownSupported: | ||
| // no All, no Single, no Own: invalid bundle | ||
| errs = append(errs, fmt.Errorf("invalid bundle: no supported install modes")) |
There was a problem hiding this comment.
This case might be confusing for users if there are operators that support only MultiNamespace install mode.
Maybe we should have a multiSupported variable and a new case for "only multiSupported", so that we can explicitly say in this function that "operators supporting only MultiNamespace install mode are not supported by OLMv1".
We might somehow preclude that elsewhere, but if we do, it seems like we should consolidate the installModes + targetNamespaces support into this function.
And then finally this case would be augmented to include !multiSupported, at which point "invalid bundle: no supported install modes" makes more sense.
| WatchNamespace: "test-namespace", | ||
| }, | ||
| expectedErr: "watchNamespace configuration parameter is not supported for bundles that only support OwnNamespace install mode", | ||
| }, |
There was a problem hiding this comment.
Related to my other comment: add a test case with the following?
installModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace}
| case allSupported && !singleSupported && ownSupported: | ||
| // All, no Single, Own: watchNamespace is optional (must == install namespace when set) | ||
| if config.WatchNamespace != "" && config.WatchNamespace != installNamespace { | ||
| errs = append(errs, fmt.Errorf("watchNamespace must equal install namespace (%s) when set for bundles supporting AllNamespaces and OwnNamespace install modes", installNamespace)) |
There was a problem hiding this comment.
Nit: Up for debate, but my instinct for clarity is to structure these error messages like this:
"bundle <bundleName> supports install modes <supportedInstallModes>: watchNamespace must be <allowedValues>"
NOTE: If the bundle name is already in some higher level error context as we return this error up the stack, then no need to repeat it here.
| errs = append(errs, fmt.Errorf("watchNamespace must equal install namespace (%s) when set for bundles supporting AllNamespaces and OwnNamespace install modes", installNamespace)) | |
| errs = append(errs, fmt.Errorf("bundle %q supports install modes [AllNamespaces, OwnNamespace]; watchNamespace must be unset or equal to the install namespace (%q)", bundle.Name, installNamespace)) |
Introduce BundleConfig that contains InstallConfig and DeploymentConfig.
Description
Reviewer Checklist