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

Moving static state to non-static #3966

Merged
merged 2 commits into from
Mar 24, 2022
Merged

Moving static state to non-static #3966

merged 2 commits into from
Mar 24, 2022

Conversation

shawkins
Copy link
Contributor

Description

This is currently layered on #3955 so it's not quite as large as it seems.

In moving the static Adapters and Handlers logic to non-static and per client, that allowed me to further refine when and how calls were made to register clients/resources. A similar change could be made for #3923 The end result is that almost all extensionadapters can be removed - they are only for the root of extensions now.

There are only 2 real client implementations with this change - the DefaultKubernetes and DefaultOpenShift. Everything else just wraps the client. Rather than expanding the ClientContext, we just pass the client around. Any modification to the client state - the config or httpclient - will make use of a copy constructor to copy over the adapters, handlers, and other state (such as the supported check overrides set by the mocking logic). Because of this change we can now move/remove mocking logic off of the annotation and onto the KubernetesServer - as any modification that we make there can be applied to any client (root or derived).

Also a lot of logic surrounding the creation of openshift clients has been greatly simplified - as most was no longer applicable.

The general strategy of Adapters / Handlers has not changed. Everything is registered as a peer. If we do want to support unregistering an extension, or doing fancier client conversion, this will need to change.

Because of the initial usage model in the extensions, I only made the override point a Resource, rather than a whole operation. There are 5 openshift operations that provide any real customization - 3 can be refactored to the existing api as they contribute only Resource methods. 2 (ProjectOperationsImpl and TemplateOperationsImpl) contribute operation level methods - which would require additional extension api changes to accommodate. So for now I added an internal interface to allow for their registration. A stretch goal would be to turn the whole openshift client into just an api extension - but I don't think there is time for that.

Also several openshift operation impls were removed by correcting whether the resource is namespaced and its plural.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@shawkins
Copy link
Contributor Author

I'll rebase and add onStart to the KubernetesMockServer reset.

@shawkins
Copy link
Contributor Author

shawkins commented Mar 22, 2022

As for moving openshift to an api only implementation:

  • Operations that contribute at the collection level, like ProjectOperations
  • Operations that read logs, there's no existing exposed method on Client for this.
  • the client logic that switches the httpclient / configuration to openshift specific. OpenShiftConfig, Readiness, and the OpenShiftOAuthInterceptor could all be moved to client to facilitate this.
  • OpenShiftNamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl - it's a whole other abstraction
  • CreateOnlyResourceOperationsImpl
  • ManagedOpenShiftClient

- converting openshift clients to extension api
- removing extension adapters as extension client adapters can act as
their own factories
- correcting namespaced/plural for several openshift resources to remove
their operations
@shawkins shawkins linked an issue Mar 22, 2022 that may be closed by this pull request
@shawkins shawkins added this to the 6.0.0 milestone Mar 22, 2022
@shawkins shawkins removed the wip label Mar 22, 2022
@shawkins shawkins marked this pull request as ready for review March 22, 2022 19:45
@shawkins
Copy link
Contributor Author

Marking this as ready for review.

#3966 (comment) - can be split as a follow up issue. It's a non-trivial amount of work to support the openshift client as api only.

I'll also follow up on the static nature of deserialization with #3972

@@ -36,19 +35,15 @@
/**
* To be used as the base class for creating extension clients
*/
public abstract class ClientAdapter<C extends Client> implements Client {
public abstract class ClientAdapter<C extends ClientAdapter<C>> implements Client {
Copy link
Member

Choose a reason for hiding this comment

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

In OperationContext a new method

  <T extends HasMetadata, V extends VisitableBuilder<T, V>> ResourceHandler<T, V> getHandler(T item) {
    return getClient().adapt(BaseClient.class).getHandlers().get(item, getClient());
  }

Is this something we might want to replicate here so that we can reuse in the DSL clients?

  Handlers getHandlers() {
    return getClient().adapt(BaseClient.class).getHandlers();
  }

or even:

... getNamespacedHasMetadataCreateOnlyOperation(...) {}
... getNonListingOperation(...) {}

Or maybe it's just not worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this something we might want to replicate here so that we can reuse in the DSL clients?

I'm trying to not expose Handlers to the api. There are only 2 usages of getHandler - to get a Builder, and to create an operation in the same context as the current.

For the Builder I'd much prefer updating the model generation logic to do something similar to lombok - add a asBuilder() method / interface to each class. Only if the resource does not implement Buildable, would you try to lookup the builder via the classpath.

Creating an operation in the same context as the current - that is only currently needed by the resourceList operations as you can do resourceList(...).dryRun().resources(). With the ExtensibleResourceAdapter that would only be needed if you override the behavior of a mutating method (create, edit, etc.) in a way that wanted to do a modification on an outside resource (none of the logic does this yet). If you just add a new method they are not available to be called after a dsl method like dryRun.


private NamespacedKubernetesClient delegate;
private DefaultKubernetesClient delegate;
Copy link
Member

@manusa manusa Mar 24, 2022

Choose a reason for hiding this comment

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

Maybe it might make sense to expose the adapters or registration capabilities through an interface (Client) and we don't have to depend on the specific type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Client methods are currently re-exposed on every Client subclass, I was wanting to keep the methods there to a minimum. I'll update the managed openshift one though to just use OpenShiftClient.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 19 Code Smells

28.2% 28.2% Coverage
7.8% 7.8% Duplication

@manusa manusa merged commit 1034e6c into fabric8io:master Mar 24, 2022
@linnjason678
Copy link

Shawkins,
I understand some items ,
Are you trying to help" a person like myself " why would you want to make so many change s so in one
Day to see if your correct or 2 see if
What if we did this or what if we did that ? I m learning and not being
Smart butt towards you and anyone
Else of their teaching s.

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.

Move static state to non-static
3 participants