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.

Sign up for GitHub

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

Jump to bottom

Support for javaType and existingJavaType extensions in java-generator #5844

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Support for javaType and existingJavaType extensions in java-generator #5844

merged 2 commits into from
Apr 9, 2024

Conversation

matteriben
Copy link
Contributor

@matteriben matteriben commented Mar 30, 2024 •
edited
Loading

Support javaType and existingJavaType extensions in java-generator

Fix #5860

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

@matteriben matteriben mentioned this pull request Mar 30, 2024
Closed
@matteriben matteriben force-pushed the iss_5843 branch 6 times, most recently from e76a2d3 to e5e09ba Compare March 30, 2024 04:13
@andreaTP
Copy link
Member

cc. @iocanel this might help with classes deduplication you have been asking for.

The downside is that, with this approach, you need to be in control of the CRDs ... still seems like a low effort way to start to close the gap.

@matteriben matteriben marked this pull request as draft March 30, 2024 16:39
@matteriben matteriben marked this pull request as ready for review March 30, 2024 16:49
@matteriben matteriben force-pushed the iss_5843 branch 2 times, most recently from ebdad5f to 5207b13 Compare March 30, 2024 17:16
andreaTP
andreaTP requested changes Apr 1, 2024
View reviewed changes
Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

@matteriben thanks a lot for starting this effort, I added some notes but I'm supportive to this effort 👍

I have a couple of additional questions:

  • I'm not 100% convinced about having 2 different options, I think that existingJavaType is enough for 90% of the real-world use cases. Applying the Okkam razor we should only keep one, happy to be convinced otherwise though 🙂
  • I'm interested in exploring(we can do this in a follow-up PR) how to achieve the same result without touching the original CRD, e.g. using a java-generator configuration option. It doesn't need to be super "easy" as it's an advanced usage, maybe re-using a logic similar to packageOverride?

Happy to have @manusa feedback here 🙏

java-generator/core/src/main/java/io/fabric8/java/generator/nodes/AbstractJSONSchema2Pojo.java Outdated Show resolved Hide resolved
java-generator/core/src/main/java/io/fabric8/java/generator/nodes/AbstractJSONSchema2Pojo.java Outdated Show resolved Hide resolved
java-generator/core/src/main/java/io/fabric8/java/generator/nodes/AbstractJSONSchema2Pojo.java Outdated Show resolved Hide resolved
...ons/src/generated/java/io/fabric8/kubernetes/api/model/apiextensions/v1/JSONSchemaProps.java Outdated Show resolved Hide resolved
@matteriben matteriben requested a review from andreaTP April 3, 2024 02:28
andreaTP
andreaTP approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

Amazing work @matteriben !
You already covered every aspect of it 🙂 the DX is(as expected) a little clunky but the functionality is there 👍

I would say that this fixes #5619 too.
LGTM!

java-generator/core/src/main/java/io/fabric8/java/generator/Config.java Show resolved Hide resolved
@matteriben matteriben force-pushed the iss_5843 branch 2 times, most recently from f8a8025 to 2cd8fb4 Compare April 3, 2024 18:18
@matteriben matteriben mentioned this pull request Apr 3, 2024
Closed
@matteriben matteriben force-pushed the iss_5843 branch 3 times, most recently from 3cef873 to d62e26a Compare April 3, 2024 20:04
@matteriben
Copy link
Contributor Author

@andreaTP @manusa @oscerd @rohanKanojia @shawkins @sunix

Please let me know if there is anything else I can do to get this change approved and merged.

@andreaTP
Copy link
Member

andreaTP commented Apr 8, 2024

@matteriben sorry for the delay, please resolve the conflicts with the changelog and I'll do a final review and eventually merge tomorrow morning.

@fabiobrz fabiobrz mentioned this pull request Apr 9, 2024
Closed
11 tasks
andreaTP
andreaTP approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

sorry for the delay and thanks a lot for this effort, really appreciated!
Great job here 👍
LGTM

java-generator/core/src/main/java/io/fabric8/java/generator/Config.java
@@ -64,6 +66,7 @@ public Config(
}
}

@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

tnx 👍

@andreaTP
Copy link
Member

andreaTP commented Apr 9, 2024

As the CI is failing on OpenShift I'm unable to merge this PR, would you mind pressing the button @manusa ?

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Apr 9, 2024

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@manusa manusa added this to the 6.12.0 milestone Apr 9, 2024 — with automated-tasks
manusa
manusa approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@manusa manusa merged commit a1d5805 into fabric8io:main Apr 9, 2024
17 of 19 checks passed
@andreaTP andreaTP mentioned this pull request Apr 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohanKanojia rohanKanojia rohanKanojia approved these changes

@andreaTP andreaTP andreaTP approved these changes

@oscerd oscerd oscerd approved these changes

@manusa manusa manusa approved these changes

@shawkins shawkins Awaiting requested review from shawkins shawkins is a code owner

@sunix sunix Awaiting requested review from sunix

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.12.0
5 participants
@matteriben @andreaTP @manusa @oscerd @rohanKanojia