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

Confusing hard-coded v1 expectation for CRD Yamls #2561

Closed
tombentley opened this issue Oct 27, 2024 · 7 comments · Fixed by #2569 or #2574
Closed

Confusing hard-coded v1 expectation for CRD Yamls #2561

tombentley opened this issue Oct 27, 2024 · 7 comments · Fixed by #2569 or #2574
Assignees

Comments

@tombentley
Copy link

Bug Report

What did you do?

I wrote a reconciler test along the lines of the one generated by the quickstart. The one difference between the quickstart and what I'm trying to do is I want to use a CRD-first approach (rather than Java-first approach) for my operator. So I'm generating by CRD classes using the Fabric8 plugin. This seems to be working fine.

What did you expect to see?

I expected to be able to use the LocallyRunOperatorExtension, maybe needing to tell it where my CRD YAML lives.

What did you see instead? Under which circumstances?

The test dies with this stacktrace:

java.lang.IllegalStateException: Cannot apply CRD yaml: /META-INF/fabric8/kafka-proxies.kroxylicious.io-v1.yml

	at io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension.applyCrd(LocallyRunOperatorExtension.java:192)
	at io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension.applyCrd(LocallyRunOperatorExtension.java:169)
	at io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension.before(LocallyRunOperatorExtension.java:157)
	at io.javaoperatorsdk.operator.junit.AbstractOperatorExtension.beforeEachImpl(AbstractOperatorExtension.java:148)
	at io.javaoperatorsdk.operator.junit.AbstractOperatorExtension.beforeEach(AbstractOperatorExtension.java:79)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.IllegalStateException: Cannot find CRD at /META-INF/fabric8/kafka-proxies.kroxylicious.io-v1.yml
	at io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension.applyCrd(LocallyRunOperatorExtension.java:180)
	... 6 more

The expectation that my CRD lives in /META-INF/fabric8/ is a bit annoying, but bearable.
A bigger problem is this seems to assume that my CRD is supporting a v1, which is not the case (I'm starting with a v1alpha1).
There doesn't seem to be any way to override this behaviour. So it looks like I'm forced to declare my CRD in a file with a really confusing name.

Environment

N/A

Possible Solution

Allow passing the CRD resource name in the call to LocallyRunOperatorExtension.withReconciler() would appear to be a reasonable API for allowing people to use the extension without hardcoding naming assumptions about CRD resources.

Additional context

@csviri
Copy link
Collaborator

csviri commented Oct 27, 2024

Thx! we will take a look, pls let is know if you plan to create a PR for this on your own :)

@csviri csviri self-assigned this Oct 27, 2024
@tombentley
Copy link
Author

Sorry, but I don't have time right now to provide a PR for this myself.

I guess another possible explanation for the v1 is it's referring to the version of CustomResourceDefinition. But if so then such a naming scheme is still pretty weird because it includes the group name of one API but the version of another.

@csviri
Copy link
Collaborator

csviri commented Oct 28, 2024

I think we just simply don't have this properly covered for tests, since mostly users use the CRD generator from java classes, will add support for this soon.

@metacosm
Copy link
Collaborator

Yes, the v1 refers to the CRD spec version, not your Custom Resource version. The other possible value is v1beta1. I agree that this is confusing and should probably include the CR version as well.

tombentley added a commit to tombentley/kproxy that referenced this issue Oct 29, 2024
tombentley added a commit to tombentley/kproxy that referenced this issue Oct 31, 2024
tombentley added a commit to kroxylicious/kroxylicious that referenced this issue Oct 31, 2024
This is based on the existing kubernetes-examples/network-topologies/portperbroker_plain mainfests.
It's also based on the JOSDK's getting started, applying the same patterns.
One point of difference with JOSDK is this uses a CRD-first approach, generating the Java CR classes using Fabric8's maven plugin. The CRD in `META-INF/fabric8` is the SoT for now. See operator-framework/java-operator-sdk#2561
That works because JOSDK uses Fabric8's Kubernetes client.
One implication of that is we end up with classes which depend on the CRD schema version.
So when we eventually evolve our CRD schema (e.g. add a v1beta1) we'd end up with a whole other class hierarchy.

Signed-off-by: Tom Bentley <[email protected]>
metacosm added a commit that referenced this issue Nov 6, 2024
This is useful when using a contract-first approach where the Java
classes are generated from the CRD instead of the reverse.

Fixes #2561

Signed-off-by: Chris Laprun <[email protected]>
@metacosm
Copy link
Collaborator

metacosm commented Nov 6, 2024

@tombentley Could you try the #2569 PR and let us know if this addresses your issue?

metacosm added a commit that referenced this issue Nov 6, 2024
This is useful when using a contract-first approach where the Java
classes are generated from the CRD instead of the reverse.

Fixes #2561

Signed-off-by: Chris Laprun <[email protected]>
@tombentley
Copy link
Author

@metacosm #2569 works for me. Thanks!

@csviri
Copy link
Collaborator

csviri commented Nov 14, 2024

Hi added an additional PR to simplify the API, hope it helps.

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