fix 3922: refining the behavior of adapt and isAdaptable #3955
Conversation
changes the mock annotation to use exclusion patterns assumes that we can change the behavior of adapt to skip checks
|
I think we probably need to make a few more refinements. Before, and after this change, if you did something like isAdaptable(V1alpha1APIGroupClient.class) - it will always return true regardless of whether Tekton is installed. I put in the javadocs that you shouldn't use the supports call with Clients, but instead with KubernetesResources. It may make more sense to just separately have a supportsClient as a replacement for isAdaptable and have supports be on be only for KubernetesResources. Then change the exetensionadapter.isSupported to not just be a boolean, but to return something like unknown by default so that we can differentiate what is actually checking for support. |
|
This next commit takes away the isSupported responsibility from ExtensionAdapters - since it wasn't really being used as intended. What I'm looking for moving forward is that users will mostly call supports or hasApiGroup to determine what is supported. This makes ExtensionAdapters almost unnecessary - if we deprecate all no-arg client constructors, then eventually you could service load the clients directly (along with the ExtensibleResourceAdapters of which there are only a handful currently). |
|
SonarCloud Quality Gate failed. |
| public <C extends Client> Boolean isAdaptable(Class<C> type) { | ||
| // if type is an instanceof SupportTestingClient, then it's a proper | ||
| // test, otherwise it could be legacy support on an extension client | ||
| C toTest = adapt(type); | ||
| if (toTest instanceof SupportTestingClient) { | ||
| return ((SupportTestingClient) toTest).isSupported(); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I'm confused. My assumption was that isAdatpable in v6.0 would return true as long as a specific client implementation for that client class exists:
For example:
falseif openshift-client-api dependency is present, openshift-client isn't and method is invoked with client.isAdaptable(OpenShiftClient.class)
However, here I'm unsure if we are also checking isSupported once the client is adapted.
There was a problem hiding this comment.
adapt will no longer perform any checks - previously at least for the OpenShift it did, which caused problems in mock scenarios.
isAdaptable I'm proposing to deprecate, but leave functionally the same - for select extensions it will call isSupported, which will do a check of the apigroups (which used to be done against the rootapis).
5.x style:
if (client.isAdaptable(OpenShiftClient.class)) {
client.adapt(OpenShiftClient.class);
...
}
That will still function the same with these changes.
6.x style:
OpenShiftClient osclient = client.adapt(OpenShiftClient.class);
if (osclient.isSupported()) {
}
Beyond that you are supposed to use:
client.supports(SomeResource.class);
To determine down to a type level what is supported.
There was a problem hiding this comment.
To determine down to a type level what is supported.
I should point out that we are effectively caching the results of those calls when the type is supported, because it backs into the same logic as client.resources(SomeResource.class) - and we never timeout those entries.
There was a problem hiding this comment.
I should point out that we are effectively caching the results of those calls when the type is supported, because it backs into the same logic as client.resources(SomeResource.class) - and we never timeout those entries.
Yes, I saw that.
My question was more regarding a newer behavior for the isAdaptable method. So in the case I'm pointing out, what will be the result (I adapt to a non-existing client implementation ---> client.adapt(OpenShiftClient.class)
There was a problem hiding this comment.
If you have the api dependency, but not the client dependency for the OpenShiftClient, both a call to adapt and isAdaptable will now throw an illegalargumentexception. That is new situation - as before there was only a single dependency.
Beyond this, with more work beyond #3966 it's possible that we can merge the openshift api and client back together as it could be written to just depend upon the client.
|
One more thought on this pr. The XXXMockServer classes are still using their default logic to create clients - this will not construct a client with the overrides on the support checks, so they will not support isAdaptable calls - but that is not necessary because the test will already be referencing the client of the expected type. |
| * | ||
| * @return the list of unsupported patterns | ||
| */ | ||
| String[] unsupported() default {}; |
There was a problem hiding this comment.
Do you have any use-case in mind for this?
Description
The idea here is to consolidate all checking logic on the apigroups/apiresources methods that was added over the last few releases.
To this end isAdaptable and supportsApiPath have been deprecated. supports replaces isAdaptable and accepts either a Client or a KubernetesResource. When using a resource it reuses the same Handler logic that can consult the api server. All other extension checks have been consolidated to hasApiGroup that can check for a single apiGroup, or matching any apiGroup. The mock logic can override calls to hasApiGroup and supports and disable whatever is desired.
In subsequent releases we may deprecate asking about Client supports entirely - and instead have a targeted check just for openshift.
adapt will perform no check of support, it will simply change the class if needed/possible. If it seems like an undersirable breaking change we can introduce a new method for this purpose and still have adapt call isAdaptable/supports
How this looks:
@EnableKubernetesMockClient() // all extensions are supported, any calls to isAdaptable or supports return true.
@EnableKubernetesMockClient(unsupported = "openshift.io") // everything but the openshift extension will be supported.
Unsupported entries can use a * wildcard, or can be made specific to a single apiGroup/version.
I've stripped out a lot of the stuff from ExtensionAdapters to the point where most of them could be replaced by just a reference to the client class - except we don't support a do-nothing no arg constructor on clients. We could of course deprecate all of the direct usage of any client (built-in or extension) constructor.
Type of change
test, version modification, documentation, etc.)
Checklist