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

feat(model): use Object instead of KubernetesResource for raw types #6333

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

manusa
Copy link
Member

@manusa manusa commented Sep 5, 2024

Description

Fixes #6332

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

@manusa manusa marked this pull request as ready for review September 5, 2024 10:22
@manusa manusa added this to the 7.0.0 milestone Sep 5, 2024 — with automated-tasks
@manusa manusa force-pushed the feat/openapi-generic branch 2 times, most recently from 5a1306f to 1a9ec4c Compare September 5, 2024 10:44
@shawkins
Copy link
Contributor

shawkins commented Sep 5, 2024

Just wanted to talk broadly on the usage of AnyType, RawExtension, GenericKuberenetesResource, and what is being done here with object and the KubernetesDeserializer.

AnyType - can be a replacement for where we had previously been using JsonNode and Map types, but is not currently directly used in our models. Also serves as a base type for RawExtension and IntOrString. Is not directly needed by the KubernetesDeserializer.
RawExtension - provides a way to bridge between KubernetesResource and any content. This is due to the previous mapping of kube's raw extension type to KubernetesResource. Generally not expected to appear directly in user object models. Used directly by the KubernetesDeserializer when a KubernetesResource is not an object or has no api/version / kind information. The kube docs imply that raw extension only allows for complete kube resources, but the schema is not actually restricted to that.
GenericKubernetesResource - generally provides a way to represent kube resource for which there is no class definition. Like RawExtension, but restricts to HasMetadata and not deserialize to the real type - only generic.

v2 CRD Mappings:
AnyType - untyped property that preserves unknown.
RawExtension - maps to raw
KubernetesResource - maps to object - this is probably not correct, if the field is exactly KubernetesResource it should map to raw instead.
GenericKubernetesResource / HasMetadata - maps to raw.

It's clear that these mappings are not bi-directional (I haven't check if the logic knows about AnyType), so round-tripping can result in a different object model.

With this pr we're mapping kube's raw extension to Object directly with the KubernetesDeserializer. If we're good with this approach we can consider deprecating the RawExtension class and recommending instead that users with custom resources take the same approach. The one thing that will look bad about this is KubernetesDeserializer is in an internal package. We'll also need to update the CRD generator to understand this convention. Also if user for whatever reason a user puts an untyped value in the Object field, it will be deserialized as a RawExtension - which doesn't work well if we want to put that on the path to deprecation.

We should also update the CRD generation logic to understand this convention, and consider making a similar change to the java generator.

AnyType could similarly be deprecated if we consider its usage synonomous with Object without the KuberenetsDeserializer - currently that will just map to an object property without preserve uknown.

Signed-off-by: Marc Nuri <marc@marcnuri.com>
@manusa manusa force-pushed the feat/openapi-generic branch from 1a9ec4c to dc2dcde Compare September 5, 2024 13:52
@manusa
Copy link
Member Author

manusa commented Sep 5, 2024

I'm extracting your comment into an issue for further discussion (especially for CRDv2).
I need to merge this to be able to proceed with the rest of model conversions.

If we're good with this approach we can consider deprecating the RawExtension class and recommending instead that users with custom resources take the same approach

I'm not 100% sure that removing the intermediate RawExtension class will allow for a complete deserialization-serialization roundtrip (this is working now, see tests in kustomize module -and others-)

@shawkins
Copy link
Contributor

shawkins commented Sep 5, 2024

I'm not 100% sure that removing the intermediate RawExtension class will allow for a complete deserialization-serialization roundtrip (this is working now, see tests in kustomize module -and others-)

It won't currently because of RawExtension's usage in the KubernetesDeserializer, but it's not needed if we fully embrace the usage of Object with KubernetesDeserializer.

Copy link

sonarqubecloud bot commented Sep 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
76.7% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@manusa manusa merged commit b5ea02b into fabric8io:main Sep 5, 2024
20 of 21 checks passed
@manusa manusa deleted the feat/openapi-generic branch September 5, 2024 14:52
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.

Usage of KubernetesResource in models creates massive builder classes
4 participants