⚠️ Only allow bundles with AllNamespaces install mode
#924
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Mikalai Radchuk <[email protected]>
m1kola
force-pushed
the
test_non_AllNamespaces_bundles
branch
from
28dae5c
to
90be17c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #924 +/- ##
=======================================
Coverage 77.38% 77.38%
=======================================
Files 17 17
Lines 1163 1163
=======================================
Hits 900 900
Misses 183 183
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -37,3 +37,29 @@ func TestClusterExtensionPackagesWithWebhooksAreNotAllowed(t *testing.T) { | |||
| assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "package-with-webhooks.1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) | |||
| }, pollDuration, pollInterval) | |||
| } | |||
|
|
|||
| func TestClusterExtensionPackagesWithUnsupportedInstallModesAreNotAllowed(t *testing.T) { | |||
There was a problem hiding this comment.
Is there a way to put this in a unit test of the ClusterExtension reconciler instead?
There was a problem hiding this comment.
@joelanford I started working on this (and another test above TestClusterExtensionPackagesWithWebhooksAreNotAllowed) when rukpak was a separate thing running in the cluster with unpacker and all that complicated setup.
The idea was that we have an e2e test which helps us verify current limitations. I was hoping that this test will also help with the helm/rukpak integration into operator-controller. But it got blocked by helm/rukpak integration :)
I do not think that we can test it on ClusterExtension reconciler: rukpak stuff is mocked there.
We can probably test it on this level with a fake FS, but at this point I'm not sure how useful it will be.
Now that we use rukpak as a library - we should probably treat it as such: assume that this stuff if well tested on the rukpak side and do not double test on operator controller.
What do you think?
There was a problem hiding this comment.
Now that we use rukpak as a library - we should probably treat it as such: assume that this stuff if well tested on the rukpak side and do not double test on operator controller.
I think with @varshaprasad96 's change in #928, where the actual call to convert the bundle happens fully in rukpak, I agree.
| @@ -24,7 +25,7 @@ const ( | |||
| ) | |||
|
|
|||
| func HandleClusterExtension(_ context.Context, fsys fs.FS, ext *ocv1alpha1.ClusterExtension) (*chart.Chart, chartutil.Values, error) { | |||
| plainFS, err := convert.RegistryV1ToPlain(fsys, ext.Spec.InstallNamespace, nil) | |||
| plainFS, err := convert.RegistryV1ToPlain(fsys, ext.Spec.InstallNamespace, []string{metav1.NamespaceAll}) | |||
There was a problem hiding this comment.
It looks like we no longer needs this fix because we no longer have it in operator-controller since #928
|
Closing as obsolete: we no longer need this fix #928. Also as discussed in #924 (comment) we do not want to double test. |
Description
TBD
Closes #816
Reviewer Checklist