fix: add clean up of applied CRDs after tests

[xstefank]

Before:

$ mvn clean install -Pintegration-tests
...

$ kubectl get crds | wc -l
83

With this PR:

$ mvn clean install -Pintegration-tests
...

$ kubectl get crds | wc -l
No resources found
0

[csviri]

@xstefank one of the E2E tests is failing (not flaky) could you pls take a look on that?

[metacosm]

That seems like a very long timeout… Do we need that much? Also maybe make it a constant, using underscores for readability?

[xstefank]

OMG, thanks, forgot this from testing the issue we discussed.

[csviri]

Wonder if this should be an opt-in feature, but no strong opinion.

[xstefank]

@csviri I also thought about it, so I can make that as a follow-up PR.

[metacosm]

Personally, if anything, I think it should be opt out. Tests should leave as little behind them as possible so this should be activated by default. I'm not even sure when you wouldn't want the CRDs removed after the tests…

[csviri]

I'm not even sure when you wouldn't want the CRDs removed after the tests…

These tests are running on ephemeral environment so at the end does not matter that much.

On the other hand adding and removing CRDs in practice does not happen that dynamically actually. Usually when cluster is pinned up CRDs an controllers are added, and rarely removed at production. Maybe when an some alternative found for some controller. But think about operators as capabilities of a platform, you might deprecate a capability but especially on large platforms (with lots of teams) almost never removed.

So yeah, there are two ways to think about this, as a java developer or as a platform engineer :)

[metacosm]

I'm not even sure when you wouldn't want the CRDs removed after the tests…

These tests are running on ephemeral environment so at the end does not matter that much.

Says who? People might run these tests locally. I know I do when I see them failing in CI and having to clean up after them is tiring :)

On the other hand adding and removing CRDs in practice does not happen that dynamically actually. Usually when cluster is pinned up CRDs an controllers are added, and rarely removed at production. Maybe when an some alternative found for some controller. But think about operators as capabilities of a platform, you might deprecate a capability but especially on large platforms (with lots of teams) almost never removed.

So yeah, there are two ways to think about this, as a java developer or as a platform engineer :)

We're only talking about the test support here, though. So it's not a matter of how CRDs should be handled in production. That's a completely different story. Here, we simply want to not leave a mess after the tests are run, if they run on a local developer cluster, and not simply in CI.

[csviri]

Says who? People might run these tests locally. I know I do when I see them failing in CI and having to clean up after them is tiring :)

Not sure if following, are you ever re-running on the same kind / minikube instance when tests failed? I mean when running the whole test suit? So basically the CRD should not matter event in that case.
But if something goes wrong you see the problem, a never try to cleanup things just run `kind delete cluster && kind create cluster' ant start with a fresh one.

But again I'm fine with this, if this helps anybody let's have it!
(Just had a feeling that I'm missing something, since never had a need for it :) )

[metacosm]

Says who? People might run these tests locally. I know I do when I see them failing in CI and having to clean up after them is tiring :)

Not sure if following, are you ever re-running on the same kind / minikube instance when tests failed? I mean when running the whole test suit? So basically the CRD should not matter event in that case. But if something goes wrong you see the problem, a never try to cleanup things just run `kind delete cluster && kind create cluster' ant start with a fresh one.

That's your use case, not mine. I don't use kind. So yes, I don't destroy my cluster after each test so I expect the tests to clean after themselves. That's just good practice.

But again I'm fine with this, if this helps anybody let's have it! (Just had a feeling that I'm missing something, since never had a need for it :) )

[xstefank]

Exactly what @metacosm said, tests should clean after themselves. I noticed this with minikube, which is not so fast to delete & restart. Of course, it could be cleaned manually, but that's precisely the point: tests should not leave any resources hanging in the void.

[csviri]

Sure, as I said agree, until it helps anybody and does not have any side effects, we should have it. (Of course I agree with the principle that if possible tests should cleanup after themself).

First of just was bit curious, since for me did not cause any issues, that I assume because the nature how CRDs are defined in K8S. Also playing a bit a devils advocate, so we have here all the arguments document at least as a form of discussion. Sorry if it look like I challenge what I should not / looks obvious :)

[csviri]

Also we could have this implemented also for ClusterDeployedOperatorExtension, so if we apply the CRD there, we might want to delete it also there.

[csviri]

Why I also though it should be opt in, since it is a change in behavior, while API is backwards compatible the behavior changes, but on the other hand I assume it won't cause issue for the users, so fine.