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] cycle in base java classes on Java 19+ #5907

Closed
wants to merge 3 commits into from

Conversation

andreaTP
Copy link
Member

Description

Proposal to attempt to fix #5866

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

@@ -94,8 +102,14 @@ public SwapResult lookupAndMark(ClassRef originalType, String name) {
return new SwapResult(null, false);
}

private static boolean isIgnored(Value value) {
Copy link
Member Author

@andreaTP andreaTP Apr 16, 2024

Choose a reason for hiding this comment

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

@shawkins I think this is a little bug in the detection of unmatched, but couldn't figure out why it's happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean here at first glance - is there a test case that could highlight what you are describing?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you remove the logic of this method and return false you have a reproducer of the issue.

@andreaTP
Copy link
Member Author

A possible next step would be to define a List of DEFAULT_SCHEMA_SWAPS to be applied always ... but I want feedback on this idea as I'm not 100% convinced myself.

@shawkins
Copy link
Contributor

A possible next step would be to define a List of DEFAULT_SCHEMA_SWAPS to be applied always ... but I want feedback on this idea as I'm not 100% convinced myself.

Please don't go down this path. It still produces a schema that is not correct and adds new internal usage of schema swap that we don't need - you could just as easily handle this with a common mapping at the level of ZonedDateTime. But ultimately you'll end up doing that for a great number of classes - this is very brittle.

@andreaTP
Copy link
Member Author

Please don't go down this path. It still produces a schema that is not correct and adds new internal usage of schema swap that we don't need - you could just as easily handle this with a common mapping at the level of ZonedDateTime. But ultimately you'll end up doing that for a great number of classes - this is very brittle.

Fair assessment, should we keep it as "manual" workaround?
Do we have other alternatives?

Open to ideas!

@shawkins
Copy link
Contributor

Fair assessment, should we keep it as "manual" workaround?

The manual workaround is to a swap on the entire ZonedDateTime.

Do we have other alternatives?

As mentioned in the other PR we can either use Jackson's schema logic - which will work automagically most of the time and keep redundant annotations to a minimum. Or we can error out if we don't think we'll produce the approriate schema. There are two paths to that - either we still use Jackson's schema logic and detect when it has something other than an object, or start blessing or marking packages / classes as supported - most of the problems are going to be with java / javax. That latter one feels too britle as well.

@andreaTP andreaTP changed the title Issue 5866 [crd-generator] cycle in base java classes on Java 19+ Apr 16, 2024
@andreaTP
Copy link
Member Author

The manual workaround is to a swap on the entire ZonedDateTime.

I'm ok with this workaround, the alternatives seem to bring additional complexity, but happy to hear more opinions/feedback/proposals.

@andreaTP
Copy link
Member Author

Superseded by #5877

@andreaTP andreaTP closed this Apr 23, 2024
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.

CRD generation fails when compiled using java version 19 and above
2 participants