-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix (extensions) : isSupported doesn't check all of the applicable API Groups (#4447) #4631
Conversation
@rohanKanojia @manusa the intent moving forward was to only expose the explicit support testing for the openshift client - Line 22 in db396dc
For the extensions, this is supposed to be an implicit mechanism that provided the same functionality as how their adapter logic used to call ExtensionAdapterSupport.isAdaptable. I was thinking in future releases we'd simply remove these checks. Since the extensions can be multi-group / multi-version clients, it's best for the user to check for explicit support of a resource. |
I'm not sure what happened here. I also feel kind of uncomfortable with the usage of an interface in production code that is specific for testing purposes. Steven, could you please give a few bullet points on how we should move forward with the interface and its usage (more specific tasks). |
We mostly wanted Client.isAdaptable to go away - it has been deprecated. The client impls that implemented SupportTestingClient did so to support checking their support via Client.isAdaptable. The client interfaces that extend SupportTestingClient, which I think is just the OpenShiftClient, are supposed to have the isSupported as part of their public contract. Rather than doing:
you are supposed to do:
There are several rationales:
|
I think these extensions also use isAdaptable method: kubernetes-client/extensions/camel-k/tests/src/test/java/io/fabric8/camelk/test/AdaptTest.java Line 33 in f6902ec
I added interface because i noticed isAdaptable is deprecated and I could not find a way to do equivalent using new adapt method. What should be expected behavior of adapt method if underlying cluster doesn't support extension's apiGroup. Should it return null if it doesn't support? |
adapt doesn't care about the cluster having support for a given API group, isSupported should be used instead. |
isSupported method is not available in current extension interfaces (KnativeClient, TektonClient etc.) . |
I was asking more about the future steps with regard to the In any case, my suggestion for the current PR is just to make the mentioned checks not exact. Then do whatever we want with the isAdaptable legacy stuff in the scope of a more detailed issue. |
SupportTestingClient.isSupported is only used in Client.isAdaptable BaseClient implementation which is deprecated, thus the interface is deprecated too. For 4447 the only requirement is to make the implementation on the default extension client implementations not exact. In a nutshell, the following statement is NOT OK:
We can later consider getting rid altogether of the interface and whatever implementation remains, but I would rather do that in the scope of a different issue where we can actually discuss the implications. |
Yes, and I think we want to keep it that way. The openshift client support check is more unique - it's primarily checking the url, then looking for any openshift group. There's a case to be made that such a broad check is not needed there either - or at the very least needs more documentation. As reported in #4604 you can just as easily have a regular kubernetes cluster return true from this check by having any of the openshift crds/operators installed - but it's certainly not actually a general openshift cluster. So rather than doing:
you are supposed to check down to a resource level:
We can of course also entertain adding client.resource(class).supports(). |
705ba06
to
7c99883
Compare
…I Groups (fabric8io#4447) + Disable exact apiGroup match in isSupported method call in extension clients which use more than one apiGroup. This effects Istio, Knative and Tekton extensions that use more than one apiGroups Signed-off-by: Rohan Kumar <rohaan@redhat.com>
7c99883
to
82fcb94
Compare
Kudos, SonarCloud Quality Gate passed! |
@manusa in the last meeting we talked about whether it would make more sense here just to go ahead and remove the deprecated isAdaptable and all of these checks from the extensions. Would you rather that be a follow-on task instead? |
Yes. This should be tackled separately. I just needed to release this one so the temp fix can be removed from Quarkus. |
Description
Fix #4447
Extension client interfaces should extend SupportTestingClient rather than implementationsSigned-off-by: Rohan Kumar rohaan@redhat.com
Type of change
test, version modification, documentation, etc.)
Checklist