Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add proper default install mode derived from watched namespaces #841

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

metacosm
Copy link
Member

@metacosm metacosm commented Mar 4, 2024

Fixes #839

Signed-off-by: Chris Laprun claprun@redhat.com

@metacosm metacosm self-assigned this Mar 4, 2024
@metacosm metacosm requested a review from csviri March 4, 2024 15:37
@metacosm metacosm force-pushed the fix-install-mode branch 2 times, most recently from 35df666 to f45b978 Compare March 7, 2024 14:17
@@ -49,6 +48,13 @@ public void shouldWriteBundleForTheOperators() throws IOException {
assertEquals(FirstReconciler.REPLACES, csv.getSpec().getReplaces());
var bundleMeta = getAnnotationsFor(bundle, "first-operator");
assertEquals(BUNDLE_PACKAGE, bundleMeta.getAnnotations().get("operators.operatorframework.io.bundle.package.v1"));
assertEquals(2, csv.getSpec().getInstallModes().size());
var installMode = csv.getSpec().getInstallModes().get(0);
assertEquals("AllNamespaces", installMode.getType());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could use the constants here too? I don't think this will change so not blocking this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always debating whether or not to use constants in the tests, especially when the tests are checking compatibility with an external service or spec. In this case, if we used a constant, and the constant value changed but not the external spec, then the test would still pass even though it would now output incorrect values.

@@ -5,7 +5,7 @@
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.quarkiverse.operatorsdk.annotations.CSVMetadata;

@CSVMetadata(name = "first-operator", version = FirstReconciler.VERSION)
@CSVMetadata(name = "first-operator", version = FirstReconciler.VERSION, installModes = @CSVMetadata.InstallMode(type = "MultiNamespace", supported = false))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there should be a public wrapper around the allowed type values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean like an enum or something like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, something that we can point users to.

Fixes #839

Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSV InstallMode is not correct by default and should take namespaces information into account
3 participants