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

Raw handling #4289

Closed
shawkins opened this issue Jul 21, 2022 · 4 comments
Closed

Raw handling #4289

shawkins opened this issue Jul 21, 2022 · 4 comments

Comments

@shawkins
Copy link
Contributor

Is your enhancement related to a problem? Please describe

The mapping of raw values is inconsistent, and leads to other issues such as the loss of non-HasMetadata values when parsing as noted on #4263 and the need to use an unmatched type handlers for templates - #4034 as the objects are actually typed as raw.

A possible semantic issue - KubernetesResource is a root interface that could server for both typed and untyped values. However GenericKuberentesResource is actually a HasMetada. Since it's optional HasMetadata aspect is optional there's a case to be made that GenericKuberentesResource could be used in general for raw.

Describe the solution you'd like

Something consistent :) But to do that there will be a breaking change.

The best option is probably to always use KuberenetesResource as the mapping. There are still some issues:

  1. Using GenericKubernetesResource as the raw value type means that it is a HasMetadata (albeit optional), callers may need to check if it's actually a valid HasMetadata depending on their usage scenario. Obviously we could also introduce an intermediate RawKubernetesResource, to compensate for this.
  2. Any existing mappings to map of object will break, but we can add a convenience method (Generic|Raw)KubernetesResource.fromMap to convert - I haven't tried, but I don't think there's a simply way to just add multiple setters and have that work well with Jackson / sundrio.
  3. We'll still need some kind of specialized template handling to preserve unmatched types, unless we force the usage Generic/Raw for anything that has a unmatched parameter.

This will not address how sundrio chooses what subtypes to add to the builder methods - it will still only add those resources that are part of the module or in the buildable references.

Describe alternatives you've considered

No response

Additional context

No response

@shawkins
Copy link
Contributor Author

Primarily the KubernetesResource interface exists to associate the KubernetesDeserializer annotation. We could just as easily associated that directly with the generated model classes, rather then using a base / marker interface.

Before we more generally use the KubernetesResource interface, there is some question about generally should be a "resource" - if it really is just synonymous with HasMetadata as all base kubernetes resources are, or if should also represent the openshift "resources" that are typically not even restful, which lack metadata. This includes request objects like LocalResourceAccessReview and their respective responses.

@shawkins
Copy link
Contributor Author

shawkins commented Aug 16, 2022

There is also io.fabric8.openshift.api.model.runtime.RawExtension in the mix, which matches what was mentioned above about a specific Raw type.

Here's a refined proposal:

  • Change the mapping of RawExtension/Map to just java.lang.Object with the field marked as using KubernetesDeserializer. That is similar to the IntOrString handling.
  • introduce io.fabric8.kubernetes.api.model.runtime.RawExtension, it will have additionalProperties.
  • io.fabric8.openshift.api.model.runtime.RawExtension should be deprecated and changed to extend RawExtension
  • KuberneteDeserializer when there is no identifying information, will return a RawExtension. Otherwise it will use a GenericKubernetesResource, or the actual type. That will refine / improve the fix from fix #4206: ensuring null won't be returned from kubernetesdeserializer #4263 which now uses a GenericKubernetesResource when there is no information.

This will break a few things:

  • the raw getters will now return object instead
  • the builders won't have any subtype methods

There's not a great way to add more convenience that won't break other usage patterns.

@manusa
Copy link
Member

manusa commented Aug 23, 2022

As a major breaking change, in a future major release (7.x) all of the classes that currently implement KubernetesResource could extend RawExtension.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 8, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 8, 2022
@shawkins shawkins mentioned this issue Sep 8, 2022
11 tasks
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 13, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 20, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 20, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 22, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 23, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 23, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 27, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 28, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 28, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 29, 2022
@manusa manusa closed this as completed in 60c2800 Oct 5, 2022
@shawkins
Copy link
Contributor Author

shawkins commented Oct 5, 2022

With #4289 committed the only follow on I can think to consider is if RawExtension is added as a buildable target (either to all or selectively to classes using a relevant mapping), that would allow for methods like:

new NamedExtensionBuilder().withNewRawExtensionExtension(value)...

vs

new NamedExtensionBuilder().withExtension(new RawExtension(value))...

That provides a better hint about how to create a raw KubernetesResources, but otherwise don't save the user much typing / mental load.

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