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 the uninstall test based on the fix in the product #120

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

jstourac
Copy link
Collaborator

Let's enable this test for RHOAI only right now. I will check what is happening with ODH sometimes later.

@jstourac jstourac requested review from Frawless and kornys March 10, 2024 16:06
@jstourac jstourac self-assigned this Mar 10, 2024
@jstourac jstourac requested a review from jiridanek March 10, 2024 16:07
@@ -38,12 +40,13 @@
@Step(value = "Deploy DSC", expected = "DSC is created and ready")
}
)
@Disabled("Disabled because of the https://issues.redhat.com/browse/RHOAIENG-499")
@DisabledIf(value = "isOdhTested", disabledReason = "This test needs to be modified to work on ODH.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

More like "ODH needs to be modified to work with the test" ;P

The problem isn't ODH but OLM, no? If you install with bundle, operator cannot uninstall, but if you install with OLM, the uninstall code (should) work on ODH as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure yet, I have to check with a real installation.

// Now the product should start to uninstall, let's wait a bit and check the result.
TestUtils.waitFor(String.format("the '%s' namespace to be removed as operator is being uninstalled",
OdhConstants.CONTROLLERS_NAMESPACE), 2000, 20000,
OdhConstants.CONTROLLERS_NAMESPACE), 2000, 120_000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd like a constant for this, but I'm not sure about the name and scope (just for this class, or global).

Comment on lines +113 to +114
if (Environment.PRODUCT.equals(Environment.PRODUCT_ODH)) {
LOGGER.info(String.format("Tested product is ODH - skipping removal of the '%s' namespace.", OdhConstants.OLM_OPERATOR_NAMESPACE));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will never run, due to the @DisabledIf, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, not now until we resolve this for ODH.

@@ -122,4 +144,8 @@ void deployDataScienceCluster() {
ResourceManager.getInstance().createResourceWithWait(dsci);
ResourceManager.getInstance().createResourceWithWait(dsc);
}

static boolean isOdhTested() {
return Environment.PRODUCT.equalsIgnoreCase(Environment.PRODUCT_ODH);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is intentional - I disabled this test on ODH for now.

Copy link
Collaborator

@jiridanek jiridanek left a comment

Choose a reason for hiding this comment

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

It should run in Jenkins and not break the existing jobs (ODH from bundle and RHOAI from OLM), so ok with me.

@jstourac jstourac merged commit 480171d into skodjob:main Mar 11, 2024
3 checks passed
@jstourac jstourac deleted the uninstallFix branch March 11, 2024 18:29
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.

3 participants