-
Notifications
You must be signed in to change notification settings - Fork 219
fix: misleading MissingCRDException exception #553
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
Conversation
if (404 == ke.getCode()) { | ||
throw new MissingCRDException(null, null); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find a better way to check whether the 404 occurs because of a missing CRD or for some other reason. The |
The check for the presence of the controller CRD is performed before (https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L165-L175) so I guess this is what Quarkus leverages. The issue I'm facing is when an event source is registered so yes it may be that a CRD is missing but could be an external CRD so Quarkus won't be able to apply it (I guess). In my specific case as example the missing CRD was a Strimzi one (because I set up an event source for those types) but the error reported that the CRD for my controller was missing, which was not true. |
Unfortunately, this isn't enough, which is why I added that code in the first place. However, I agree that it's subpar… I'm working on an hopefully better solution. |
Wonder if it would be simpler if Quarkus should just apply the CR if it is configured to do so without checking, what would be the downside ? |
That's a good point and I actually think that's the direction things are going now… The exception was introduced when the extension didn't have the ability to re-apply the CRD on changes but it now has that ability so we might indeed simplify things here… I will have to check. I've been working on improving the exceptions in the fabric8 client as well so will first finish on that path to validate the approach. |
+1
Agree, better exception from fabric8 client would be very useful. |
See #558 |
I guess I can close this one then ? |
I think so, yes. Just let me know if the fix in #558 works for you. :) |
LGTM |
Superseded by #558. Thanks for bringing this up! |
Fixes #552