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

Adapting api and checks performed #3922

Closed
shawkins opened this issue Mar 3, 2022 · 1 comment
Closed

Adapting api and checks performed #3922

shawkins opened this issue Mar 3, 2022 · 1 comment
Assignees
Milestone

Comments

@shawkins
Copy link
Contributor

shawkins commented Mar 3, 2022

ExtensionAdapter/ ExtensionAdapterSupport / APIGroupExtensionAdapter:

  • APIGroupExtensionAdapter is broken - could be made to work using calls to getApiResources(apiVersion) - but the getAPIGroupName as an apiVersion is not consistently specified. In some classes it's just group, others just version, and others the full apiVersion.
  • ExtensionAdapterSupport caching strategy is not generally safe, there could be a timing issue wrt the initial ask for support or you could later remove the extension. Could just be a check of getApiGroup(name) rather than investigating the rootPaths
  • In either of the above cases, a similar strategy as https://github.com/shawkins/kubernetes-client/compare/extension-api...shawkins:extension-mock?expand=1 could be used to intercept or even provide implementations for getApiGroup and getApiResources to provide mock support.
  • OpenShift is the odd man out as it checks across everything for at least one openshift. That makes it more involved to provide alternative BaseClient logic (not simply based upon the client class type).

My general question is how much of this is useful to implement / retain? Possibilities include:

  • make this work as originally envisioned - isAdaptable and adapt will both check for support at both the overall client and api level. Caching of support may need to be refined to have a timeout. It may be better to have an exclusion list on EnableKubernetesMock of supported clients, rather than an inclusion list.
  • Expanding on the previous comment - separate the notion of isAdaptable and adapt. adapt could do no checks (neither in ExtensionAdapter nor BaseClient) of support, and isAdaptable could be deprecated/renamed to isSupported - and potentially do no caching. The idea would be that clients would be making only infrequent calls to isSupported, but adapt should generally be expected to work and is called both internally and by clients.

Originally posted by @shawkins in #3876 (comment)

@manusa
Copy link
Member

manusa commented Mar 8, 2022

As agreed, we should deprecate Client#isAdaptable method and point to the new isSupported method. The new behavior of isAdaptable checks if the client can be converted/adapted to the Extension specific client implementation. With the current code-base isAdaptable resulting in false would only be possible in the case of OpenShift: a user has a dependency to the OpenShift Client API, but not to the OpenShift Client module (the call client.isAdaptable(OpenShiftClient.class) or client.adapt(OpenShiftClient.class) would fail).

The isSupported method checks if the configured cluster actually supports the Extension (is OpenShift, has the required CRDs deployed, etc.). The value is not cached, so for every invocation the check is performed.

@shawkins shawkins self-assigned this Mar 8, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 9, 2022
changes the mock annotation to use exclusion patterns

assumes that we can change the behavior of adapt to skip checks
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 10, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 10, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 10, 2022
@manusa manusa closed this as completed in bb6115a Mar 22, 2022
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

No branches or pull requests

2 participants