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

CRD generator implementation that supports Jakarta and Swagger #6315

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adriansuarez
Copy link

Description

This change adds an alternative CRD generator implementation that is based on
victools/jsonschema-generator.

The victools generator allows modules to be registered to customize schema generation, and there are modules that provide support for Jakarta Validation and Swagger annotations.

These changes are being contributed into test scope as a prototype, since no effort has been made to integrate it with the existing CRD generator API. It is a totally separate implementation that is not at parity with the existing generator. The intent is that someone with more knowledge of the Fabric8 codebase can incorporate some aspects of it into the real product.

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

This change adds an alternative CRD generator implementation that is
based on
[victools/jsonschema-generator](https://github.com/victools/jsonschema-generator).

The victools generator allows modules to be registered to customize
schema generation, and there are modules that provide support for
Jakarta Validation and Swagger annotations.

These changes are being contributed into test scope as a prototype,
since no effort has been made to integrate it with the existing CRD
generator API. It is a totally separate implementation that is not at
parity with the existing generator. The intent is that someone with more
knowledge of the Fabric8 codebase can incorporate some aspects of it
into the real product.
@shawkins
Copy link
Contributor

It is a totally separate implementation that is not at parity with the existing generator. The intent is that someone with more knowledge of the Fabric8 codebase can incorporate some aspects of it into the real product.

It would be good if you could provide an overview of what doesn't work from the current generator, and if there is some other / different mechanism that could be used instead. SchemaSwap in particular seems like may be difficult to support.

@adriansuarez
Copy link
Author

adriansuarez commented Aug 30, 2024

It would be good if you could provide an overview of what doesn't work from the current generator, and if there is some other / different mechanism that could be used instead. SchemaSwap in particular seems like may be difficult to support.

It's a completely separate implementation, so nothing works that is not explicitly implemented. I added the Fabric8Module which adds support for several Fabric8 CRD annotations, but not @SchemaSwap.

The following are supported:

  • @io.fabric8.generator.annotation.Default
  • @io.fabric8.generator.annotation.Max
  • @io.fabric8.generator.annotation.Min
  • @io.fabric8.generator.annotation.Nullable
  • @io.fabric8.generator.annotation.Pattern
  • @io.fabric8.generator.annotation.Required
  • @io.fabric8.generator.annotation.ValidationRule/@io.fabric8.generator.annotation.ValidationRules

Everything else is unsupported.

The @AdditionalPrinterColumn annotation that allows specifying arbitrary JSONPaths replaces @PrinterColumn (see #3069). It would be straightforward to add support for @AdditionalPrinterColumn to the current generator or support @PrinterColumn in the new generator, but I had no use for the latter in my own code.

The important difference between this implementation and the current one is the replacement of jackson-module-jsonSchema with victools/jsonschema-generator. It should be possible to consolidate the functionality here with the current implementation by replacing the schema generator only and keeping everything else, but when I attempted that I had a hard time breaking the jackson-module-jsonSchema dependency.

@baloo42
Copy link
Contributor

baloo42 commented Aug 30, 2024

Thanks for submitting this. I will dig into it because I also have some concerns about the future of jackson-jsonSchema. Especially because of this section in the readme.

Expect some feedback from me in the next days.

@shawkins
Copy link
Contributor

but when I attempted that I had a hard time breaking the jackson-module-jsonSchema dependency.

Perhaps that would be a better starting point - could you open a pr with whatever conversion you had already gotten to? For the most part it seems like it will need to be approached similar to the v2 logic. Start with the schema conversion, then walk that structure along with the object graph to make adjustment to the final form emitted by fabric8.

Especially because of this section in the readme.

Keep in mind this does not seem to be time sensitive as Jackson 3.0 has been under works for 7 years and there are no definitive release timelines that I have found.

@baloo42
Copy link
Contributor

baloo42 commented Aug 31, 2024

Keep in mind this does not seem to be time sensitive as Jackson 3.0 has been under works for 7 years and there are no definitive release timelines that I have found.

Sure. It's definitily not a top priority, but for me it's interesting to explore because I'm more familiar with victools than jackson-jsonschema.

@adriansuarez
Copy link
Author

Perhaps that would be a better starting point - could you open a pr with whatever conversion you had already gotten to?

I didn't make any changes. I just looked at the code and saw that the jackson-module-jsonSchema dependency was bleeding into the CRD generation, and decided to not even attempt to make the schema generator configurable since it would require changing a ton of stuff.

Specifically, com.fasterxml.jackson.module.jsonSchema.JsonSchema is used in several places rather than an opaque form like JsonNode or ObjectNode.

Keep in mind this does not seem to be time sensitive as Jackson 3.0 has been under works for 7 years and there are no definitive release timelines that I have found.

The main concern for me as that the maintainers do not seem to be making improvements to the library or maintaining support for things like Jakarta EE. See the following issue, which was closed without a fix: FasterXML/jackson-module-jsonSchema#150 (comment)

The fact that victools is actively maintained and already has support for Jakarta Validation and Swagger annotations (which would solve several open issues; see #5859) seems to make it a no-brainer, though I do understand that it is work to move away from jackson-module-jsonSchema.

@shawkins
Copy link
Contributor

shawkins commented Sep 9, 2024

Specifically, com.fasterxml.jackson.module.jsonSchema.JsonSchema is used in several places rather than an opaque form like JsonNode or ObjectNode.

We're not directly emitting jackson's JsonSchema, rather we're walking that along with with the object structure and base Jackson logic to emit the final form, which is based upon the CRD object model generated by fabric8.

Please take another pass at using victools but starting instead with the current state of the v2 generator. If you run into issues, that can be discussed on the PR.

Jakarta Validation and Swagger annotations (which would solve several open issues; see #5859)

I wasn't deeply involved in earlier discussions, but the gist of what I gathered is that we've been reluctant to support additional annotations either because of the additional dependencies or the possible mismatch in semantics. So much of this support would likely end up being considered optional / plugin driven regardless.

@baloo42
Copy link
Contributor

baloo42 commented Oct 1, 2024

@adriansuarez sorry for the delay, I digged into it and started my own experiment based on your ideas which went too far. The results can be found here: https://github.com/baloo42/crd-generator-victools
Feedback welcome ;)

Regarding @AdditionalPrinterColumn please have a look at #6390.

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.

3 participants