KubernetesDeserializer class resolving with SPI

[shawkins]

Description

Fixes #3923
Fixes #4515
Supersedes closes #4024

Alternative to #4024 that Addresses #3923 by using a ServiceLoader for KubernetesResources off of a new list of resources based upon the existing manifest.vm. This greatly simplifies the logic in the KubernetesDeserializer.

On a related note, I'm not entirely clear what the legacy use of the manifest.vm and the model.properties is.

I have not introduced a replacement mechanism for the KubernetesResourceMappingProvider, other than a user creating their own KubernetesResource services file - which is breaking change. If it seems necessary, we can reintroduce the legacy KubernetesResourceMappingProvider, or something like it (see also what was proposed in the other pr).

If a mechanism like the KubernetesResourceMappingProvider is reintroduced, that could make use of jandex on a per custom model basis rather than as a core dependency. That would look like an auto-discovery module has the jandex dependency and an implementation of KubernetesResourceMappingProvider with the jandex logic. Then you just have to make that a dependency of your module / bundle (with the mapping provider services file re-exposed) and generate the jandex index for everything to work.

cc @andreaTP

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

[iocanel]

LGTM

[iocanel]

It's been so long that I can't really recall the need for model properties. I would assume though that it was added to serve code generation purposes in the client. Possibly, at some point it was needed to generate ResourceHandlers or OperationImpl.

[shawkins]

@manusa the changes have been rebased to latest. Also the tracking of resources by just kind has been removed - this means that no filtering needs to be performed prior to adding KubernetesResources. To confirm the thinking on quarkus integration were you suggesting that quarkus generate any missing KubernetesResource.properties files, or were you still expecting to use static methods, or a different spi to register classes?

Also are you good with simply dropping the old KubernetesResourceMappingProvider support, or do you want it reintroduced / deprecated here?

[manusa]

I'll start reviewing as soon as I finish this comment (i.e. my only context is description and your comments)

• KubernetesResourceMappingProvider
  I'm fine removing it as long as there is a replacement. From last week's discussion, resources are registered through Sundrio auto-generated SPI definition files legeraging the model.properties template file. I assume that extensions have this replacement in place.
• Quarkus:
  The idea is that Quarkus leverages the SPI generated descriptors so that the KubernetesDeserializer works in native mode too. This shouldn't be a problem, and nothing else should be needed (release notes should include the list of SPI interfaces that have been added/removed). Once we release, I'll update those when bumping the Kubernetes Client dependency for Quarkus.

[manusa]

LGTM

[shawkins]

I'm fine removing it as long as there is a replacement.

The replacement for the KubernetesResourceMappingProvider is to create the KubernetesResource.properties file - that isn't yet documented. I'm honestly not sure how many users would need that functionality.

I assume that extensions have this replacement in place.

Yes every model module built as part of the main project will have this file as everything uses a common annotator.

The java generator project uses a separate set of logic, that does not include .properties generation - but could because it already has a dependency on sundrio for builder generation.

The other alternative we discussed was to introduce a maven plugin that would leverage jandex to create the KubernetesResource.properties

[manusa]

I was referring exclusively to the extensions. As confirmed by your comment and reviewed in the code changes, there's a replacement in place that leverages the Sundrio @TemplateTransformation annotation

The other alternative we discussed was to introduce a maven plugin that would leverage jandex to create the KubernetesResource.properties

I don't think that this should be considered at this point.

[shawkins]

After this is committed the only name based convention still in use will be how we look for List/Builder implementations. We do have the option of addressing this if need be by introducing a List/Builder annotations, but it seems like this convention is unlikely to ever need breaking - you'd have to have a resource called Foo and FooList in the same package for there to be a conflict.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

78.9% Coverage
0.0% Duplication