Skip to content

CDI and dependent resources #685

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

Closed
shawkins opened this issue Aug 16, 2023 · 10 comments
Closed

CDI and dependent resources #685

shawkins opened this issue Aug 16, 2023 · 10 comments
Assignees

Comments

@shawkins
Copy link

Related to operator-framework/java-operator-sdk#2010 there could be an improvement in how dependent resources are usable in quarkus. Would it be possible for the quarkus extention to override how dependent resources are instantiated so they are created via CDI instead?

@shawkins
Copy link
Author

@csviri @metacosm Alternatively other than using a standalone dependent resource or the context, is there an existing hook for the controller to provide some general state to its dependent resources?

@metacosm
Copy link
Member

Related to operator-framework/java-operator-sdk#2010 there could be an improvement in how dependent resources are usable in quarkus. Would it be possible for the quarkus extention to override how dependent resources are instantiated so they are created via CDI instead?

Unless I'm misunderstanding your question, this is already the case, see: https://github.com/quarkiverse/quarkus-operator-sdk/blob/main/core/runtime/src/main/java/io/quarkiverse/operatorsdk/runtime/QuarkusConfigurationService.java#L213 and https://github.com/quarkiverse/quarkus-operator-sdk/blob/main/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/OperatorSDKProcessor.java#L185-L208

@metacosm
Copy link
Member

what sort of state and in what form? Could you maybe elaborate on your use case so we can see what's feasible or what needs to be improved?

@metacosm metacosm self-assigned this Aug 16, 2023
@shawkins
Copy link
Author

Unless I'm misunderstanding your question, this is already the case

So this is a test issue then. Keycloak is constructing an Operator per test -
https://github.com/keycloak/keycloak/blob/ec2728c55c468ce3ba29af6de4cb6a1fd15e3324/operator/src/test/java/org/keycloak/operator/testsuite/integration/BaseOperatorTest.java#L171

What should be used to construct the Operator to make it use the quarkus logic?

what sort of state and in what form? Could you maybe elaborate on your use case so we can see what's feasible or what needs to be improved?

Common application scoped beans such as a quarkus config interface instance.

@metacosm
Copy link
Member

Unless I'm misunderstanding your question, this is already the case

So this is a test issue then. Keycloak is constructing an Operator per test - https://github.com/keycloak/keycloak/blob/ec2728c55c468ce3ba29af6de4cb6a1fd15e3324/operator/src/test/java/org/keycloak/operator/testsuite/integration/BaseOperatorTest.java#L171

What should be used to construct the Operator to make it use the quarkus logic?

The Operator instance would need to be created via CDI as well, I think. Test support could definitely be improved.

what sort of state and in what form? Could you maybe elaborate on your use case so we can see what's feasible or what needs to be improved?

Common application scoped beans such as a quarkus config interface instance.

Could you give me an example, please?

@shawkins
Copy link
Author

Could you give me an example, please?

https://github.com/keycloak/keycloak/blob/main/operator/src/main/java/org/keycloak/operator/Config.java - is used in multiple places.

@shawkins
Copy link
Author

The Operator instance would need to be created via CDI as well, I think. Test support could definitely be improved.

The sticking point on this appears to be the kubernetes client namespace - trying to create one for each test class, similar to the fabric8 NamespaceExtension, does not play nicely with the assumptions in quarkus, namely that the namespace is coming from the quarkus config. So it seems like the only way to preserve this behavior would be look really hackish - a different profile per testclass that overrode the namespace.

@metacosm
Copy link
Member

The Operator instance would need to be created via CDI as well, I think. Test support could definitely be improved.

The sticking point on this appears to be the kubernetes client namespace - trying to create one for each test class, similar to the fabric8 NamespaceExtension, does not play nicely with the assumptions in quarkus, namely that the namespace is coming from the quarkus config. So it seems like the only way to preserve this behavior would be look really hackish - a different profile per testclass that overrode the namespace.

You could also use the Kubernetes devservices instead, unless you need a cluster to persist across several tests. Using the dev service would simplify the state management.

@shawkins
Copy link
Author

You could also use the Kubernetes devservices instead, unless you need a cluster to persist across several tests. Using the dev service would simplify the state management.

Currently no state is shared across test classes. The same testing logic assumes both local and remote testing. The remote may be an openshift instance for some of our test pipelines. We also have at least one non-quakus test, which is just using the fabric8 KubernetesTest which has the expectation of a running cluster.

@shawkins
Copy link
Author

Resolving as closed, I can workaround this by creating a cusomized BaseConfigurationService. We can circle back later if want to align the tests more with quarkus.

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

No branches or pull requests

2 participants