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

Move the rest of the extensions to the new api from #3845 #3876

Closed
shawkins opened this issue Feb 17, 2022 · 4 comments · Fixed by #3917
Closed

Move the rest of the extensions to the new api from #3845 #3876

shawkins opened this issue Feb 17, 2022 · 4 comments · Fixed by #3917
Milestone

Comments

@shawkins
Copy link
Contributor

Is your task related to a problem? Please describe

To keep the pr size for #3845 more manageable, this was captured as a follow on task.

Describe the solution you'd like

In addition to converting the other extensions we should also consider cleaning up ExtensionAdapterSupport, making a minimal development guide, and addressing any of the other leftover items from the initial issue/pr.

Describe alternatives you've considered

No response

Additional context

No response

@shawkins
Copy link
Contributor Author

I think we can also do away with the need for KubernetesResourceMappingProvider - that can be driven by either an annotation scan for Group/Version/Kind, or could be loaded through basically the same mechanism as what is registering handlers that is proposed by #3845. It all depends on the expectation of if the user can make use of KubernetesDesializer / Serialization independently of the client.

@shawkins
Copy link
Contributor Author

@iocanel @rohanKanojia @manusa With these changes, I'd like to propose simplifying the mocking logic for extensions - to not require an additional module. This would look like: https://github.com/shawkins/kubernetes-client/compare/extension-api...shawkins:extension-mock?expand=1

I did not try to yet minimize the duplication between KubernetesMockExtension and KubernetesMockServerExtension (that could easily be done, or we could choose to throw away the legacy mock support).

Instead of extension specific annotations and mock servers you'll just use: EnableKubernetesMock - if you don't specify the specific extensions to be supported, then any Client will automatically be provided. If you do provide the specific set of extensions this allows for you to control the behavior of tests for classes that may need tested separately as OpenShift and as base Kubernetes.

This may need some further refinement to what isAdaptable and adapt mean. For one the openshift logic is making a call to isAdaptable in adapt - the other extensions do not. I've kept that in these changes, but moved that to the BaseClient.isAdaptable. It could be that we don't need that check at all - isAdaptable could mean "isSupported", and adapt could always simply change the client type.

@shawkins
Copy link
Contributor Author

@iocanel @rohanKanojia @manusa other thoughts:

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.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 2, 2022
This also requires the use of ExtensionResourceAdapter so that an
explicit constructor is no longer required
@shawkins
Copy link
Contributor Author

shawkins commented Mar 3, 2022

Moved the last two comments to their own issues, so that this issue can be resolved with #3917

manusa pushed a commit that referenced this issue Mar 7, 2022
This also requires the use of ExtensionResourceAdapter so that an
explicit constructor is no longer required
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 a pull request may close this issue.

1 participant