Skip to content

fix: only throw MissingCRDException if we get a 404 on the target CRD #558

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

Merged
merged 2 commits into from
Sep 23, 2021

Conversation

metacosm
Copy link
Collaborator

Note that this relies on fabric8io/kubernetes-client#3492 so won't build
until this PR is merged and available in a release.
Fixes #552

@lburgazzoli
Copy link
Collaborator

LGTM

wonder if the MissingCRDException should only be thrown by the CustomResourceEventHanderl instead of any event source

@@ -92,7 +95,10 @@ public final void registerEventSource(String name, EventSource eventSource)
if (e instanceof KubernetesClientException) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not we catch this exception and make these checks in the CustomResourceEventSource?
(And just throw the exception further if this is not the exception we want to trasnlate)

@@ -92,7 +95,10 @@ public final void registerEventSource(String name, EventSource eventSource)
if (e instanceof KubernetesClientException) {
KubernetesClientException ke = (KubernetesClientException) e;
if (404 == ke.getCode()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean this logic. This is quite specific to CustomResourceEventSource. Cannot be moved there?

@metacosm metacosm force-pushed the fix-missing-crd-exception branch from 0b9cee8 to a899a4c Compare September 23, 2021 20:08
This indeed makes more sense like this as this should only happen when
attempting to register watches for the primary resources.
@metacosm metacosm marked this pull request as ready for review September 23, 2021 20:51
Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGTM!

Nice late night PR :)

@metacosm metacosm merged commit ccf3053 into master Sep 23, 2021
@metacosm metacosm deleted the fix-missing-crd-exception branch September 23, 2021 21:12
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.

Misleading MissingCRDException exception
3 participants